Re: [Chicken-hackers] Scheduler fdset assert patch

[Peter Bex]

On Tue, Jul 14, 2015 at 07:53:45AM -0400, Jonathan Chan wrote:
> Hi Peter,
> 
> Thanks for the comments!
> 
> Here is a revised version of the patch (I came up with yet more edge
> cases...) that has a very long comment fully explaining my reasoning.
> I've also ported the scheduler tests I made to pure CHICKEN so they
> should easily run on Windows and added some more tests for the new edge
> case. mutex-unlock! also didn't seem to be clearing the block object
> after unlocking, so I added one line to do that.

Hello Jonathan,

Wow, that's a long comment! Thanks for putting so much time into this.
This is not exactly what I had in mind though.  I would prefer it if
the comment wouldn't take up that much space.  For example, the
explanation of "reason" and "state" doesn't need to be three lines
of prose.  Instead, I'd simply add a short inline comment like so:

 (set! ##sys#interrupt-hook
   (let ((oldhook ##sys#interrupt-hook))
     (lambda (reason state) ; reason = signal #, state = proc or context
       ...

This is much easier on the eyes and provides the information exactly
when and where you need it while reading over the code.  Renaming the
variables to clarify their meaning might be even better, but that's
debatable in this case.  The main problem with such a big chunk of
comment up front is that it requires the reader to first read this,
memorize it, scroll down, and figure out how the code maps to the
comment.  For a high-level overview that's okay, but this is the wrong
level of description to warrant such a long comment, I think.  Cutting
it down might help.

Also, signing the comment with your name and date might not be the wisest
idea.  I've seen this kind of thing in code before, and you often see
that  people may want to edit the comments, which would distort your
original message (meaning it looks like you signed something you didn't
write).  So what usually happens in these cases is that others append
to it, creating a sort of alternate history log, which gets messy quickly.

Small nitpick: a single-semicolon comment will be auto-indented by Emacs
as a "margin" comment, which pushes it to the right.  Since most Schemers
will be using Emacs to edit this code, it's better to use two (or three,
if the comment is important enough) leading semicolons.

Invoking external programs like "pgrep" or "wmic" is brittle and if you
can, you should avoid it.  According to the documentation, PROCESS should
already return the process id as the third value, so I think you can just
use that. 

Is the split of ##sys#schedule and ##sys#schedule* needed?  I don't see
why these have to be separate procedures.

Finally, you didn't reply to my question regarding the change in
create-fdset where the original code contained the following:

  (let ((p (##sys#slot t 11)))
    (when (pair? p) ; (FD . RWFLAGS)? (can also be mutex or thread)
      (fdset-set fd (cdr p)))))

Your code is just taking the cdr without first checking it's a pair.

Cheers,
Peter

signature.asc
Description: Digital signature