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 _______________________________________________ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers