Re: [Chicken-hackers] Scheduler fdset assert patch

[Peter Bex]

On Fri, Jul 10, 2015 at 08:45:02PM -0400, Jonathan Chan wrote:
> Hello all,
> 
> Here is the scheduler assert patch, test cases, and integration with the
> test suite combined into a single patch! After a bit more testing with
> code that used a lot of threads, I ended up looking at how make-thread
> uses ##sys#make-thread and added some missing code to the patch's usage
> of ##sys#make-thread.

Hi Jonathan!

First off, thanks for your efforts to fix this nasty old bug.  This is
a very improvement to CHICKEN.

I haven't had time to study your patch in detail yet, but here are a
few first comments:

- The test expects the user to compile sink.c manually, this means a
   simple "make check" will fail.  Compilation should be added to the
   test suite for it to work without manual intervention.
- runtest.bat in Windows doesn't have the new test.
- The new files need to be added to distribution/manifest or they won't
   be included in distribution tarballs (we all forget this one :P)
- In the original version of scheduler.scm, there's a line that says
    (when (pair? p) ; (FD . RWFLAGS)? (can also be mutex or thread)
  You seem to have replaced this with an unconditional cdr in the foldl;
  is that intentional (ie, was the comment wrong)?
- The foldl is rather complicated.  Could you add some comments to the
   lambda to explain what it's doing, exactly?
- Speaking of comments, maybe adding a high-level comment that explains
   why the handler needs to be wrapped would be a good idea as well.
   The scheduler is nasty tricky code, which really needs more comments.
   Of course I understand that you've been given code without comments
   and that's a problem with the existing codebase, but now that you have
   all the info in your mind, it would be great to add back some comments
   to make it easier for the next person who needs to touch this code.

Cheers,
Peter

signature.asc
Description: Digital signature