Re: [Chicken-hackers] [MEGA-PATCH] Modularise the compiler!

[Peter Bex]

On Thu, Sep 04, 2014 at 01:02:18AM +0200, Felix Winkelmann wrote:
> Hello!
> 
> 
> I have signed off a "squash" commit of all your changes and pushed it
> to a new branch ("chicken-5"). I have a proposal for more meaningful
> module names, though, and I will post a patch in the next days.
> 
> Thanks for investing the considerable effort of figuring all of this
> out.  That is quite impressive, I must say - well done!

Thanks for taking a look at the patches, and pushing them!

I did wonder why you squashed the commits; like I said, I put a lot
of effort into making them self-contained (and every single commit
compiles and passes the testsuite).  By squashing them, it becomes
one big "blob commit".  If this commit introduces a bug, "git bisect"
will be mostly useless, whereas if the individual commits were kept
it would give us much more digestible changes.

On Thu, Sep 04, 2014 at 11:55:02PM +0200, Felix Winkelmann wrote:
> Hi, again!
> 
> A few notes, regarding the compiler-modularization patch:
> 
> * The commit messages suggest the possibility of different target
>   platforms. I think we can safely assume that the compiler will not
>   generate anything but C for the time being, so I don't think we need
>   to take precautions for that.

OK, I just wanted to put it out there, because the naming and structure
of the code seems to suggest this was a consideration once.

> * But, _if_ this should ever be intended, then I think putting the
>   analysis-DB handling into "batch-driver.scm" seems wrong (commit
>   0cbadaf978dd24e0b904ab62d400e670e2688c79)

I don't remember exactly why I did that.  Probably it accesses some
variables from the batch-driver, and the lower-level modules cannot
depend on batch-driver (that would be a cyclic dependency).

> * There seems to be a question regarding whether declarations in
>   evaluated code need to be processed. I don't recall the exact
>   reasons, but there are situations where this is needed. If you want
>   I can dig deeper into this.

Please do.  If we can rip out that hacky-looking code that would be nice.

> * Regarding the changes to pass state around instead of using globals:
>   I think this is suboptimal, since it makes the code more difficult
>   to understand (IMHO) and is more brittle, especially, if the
>   procedure signatures need to be extended. The compiler is not, and
>   doesn't need to be, structured for multiple sequential runs and the
>   passed state doesn't even change in many situations (the
>   "block-compilation" argument to "variable-visible?" or example). So
>   I think doing this is counterproductive (commit
>   b98203c1f2ef4e5e5cf8e9658fd1c70031abf1a1).

I'm a little unhappy about the many variables that float around in the
compiler.  It's a little messy, and it prevents making the compiler
available as a library (which would be cool if we ever want to make
an IDE or integrate even deeper with Emacs, or add the ability to compile
procedures on-the-fly, or whatever.  But that's probably a pipe-dream
anyway).

> * To allow the import of user-pass parameters, we need to expose at
>   least some of the import-libraries for the compiler modules.  I'm
>   not sure whether a single one might do, or whether all need to be
>   compiled and stored in the repository. Possibly the latter, since
>   code implementing user-passes might want to access the various
>   internal procedures.

I was a little unsure about that.  Like I said, I wanted to avoid
installing the import libraries just yet, because they're just the
existing stuff wrapped up as modules, not some well thought-out API.
If we install these import libs and make it an official API, we're stuck
with the way it works.

Maybe one of the first eggs to port would be "bind", as it hooks in
the compiler so deeply.  That way, we could see what kind of things are
truly needed and expose just those, as a preliminary official API.

OTOH, maybe this is just too hard to tackle right now.  However, we
can keep it semi-official by not installing the import libs; it's still
possible to refer to compiler variables by using the fully-qualified
identifiers (with the module prefix).

> * This also brings up the possibility of name-clashes between eggs and
>   compiler module names.

Yeah, that thought occurred to me too: they're very generic-sounding.

>   I would like to change the module names, for
>   example to "chicken.compiler.<module>". Attached is a patch that
>   does this. I renamed "compiler.scm" to "core.scm" to have a more
>   consistent naming of the files and modules
>   ("chicken.compiler.compiler" just sounded too confusing to me). I
>   had to change some things in the build for this, perhaps you can
>   review whether I broke anything.

I like this.  A lot!  I've pushed this.

Please note that the aforementioned declaration processing code in
eval.scm has not been changed: it still reads
"compiler#process-declaration".  However, none of our tests fail,
so either it's really unnecessary, or the testsuite is incomplete.
If it does seem to be needed, this will need to be changed to
"chicken.compiler.core#process-declaration".

> * The mentioned patch does not yet compile and install the import
>   libraries for the compiler modules.

I'd love to hear opinions from other hackers on what to do with the
compiler's import libs.

Cheers,
Peter
-- 
http://www.more-magic.net