2008/8/24 Allison Randal <[EMAIL PROTECTED]>: > Reini Urban wrote: >> >> You want one patch only against HEAD? That's easy. >> But I dislike the idea, as it violates the usage of single tickets. > > This is different than the usual case as it's a collection of dependent > patches that can't be evaluated independently. Splitting them out is > actually more work for the reviewer/tester. > > I've created a branch cygwin070patches for testing this collection. To do > platform or language testing, please check out that branch. Reini, please > submit further changes as diffs against that branch instead of updates to > your previous patch files. > > Overall the work is sane. It'll need a few changes before merging in: > > In runtime/parrot/library/config.pir you add commented-out code, and a > mention that certain logic has to be reversed "when installed versions > should run faster than source builds". Those sorts of configuration changes > should never involve commenting out and uncommenting bits of configuration > files. Make it a compile-time or run-time configuration flag instead.
Just was just an internal TODO comment, and is actually fixed. I will delete the superflous comment. > In lib/Parrot/Configure/Data.pm, you changed some double C<<>> Pod tags to > single C<> Pod tags. But, those code items contain "=>" separating the > key/value arguments, and the '>' in the arrow will terminate the code tag. > The double C<<>> tags avoid terminating on single '>' (which is why they > were double C<<>> tags in the first place). So, I reverted that file before > committing. (See the output from 'perldoc' or any Pod parser on your > modified file.) Thanks. > In lib/Parrot/Configure/Compiler.pm, I agree that 'CONDITIONED_LINE' and > 'INVERSE_CONDITIONED_LINE' aren't the clearest names, but '+' and '-' are > far less clear. Change them to something meaningful like 'SHOW_LINE_IF' and > 'HIDE_LINE_IF'. We can note the change in DEPRECATED.pod now, and remove > 'CONDITIONED_LINE' and 'INVERSE_CONDITIONED_LINE' after a standard > deprecation cycle (one release). #+ and #- are well known common lisp idioms (the most well-known reader-macros), thats why the reverse polish notation in the logic is used. It's just so much easier to parse and understand. > Also in lib/Parrot/Configure/Compiler.pm, change the Polish notation of > "(and a b (not c d))" to a saner "(a and b not (c and d))". This is ALGOL syntax you suggest is really not "sane" :) > Though really, > since you're not implementing the advanced conditions, delete the comment > defining the interface and the TODO comments about implementing it, just add > a TODO RT ticket or keep it on your private TODO list. I'll implement it for sure, but later. First I wanted to bring out the packages and this is just convenience sugar. > In config/gen/makefiles/pge.in, config/gen/makefiles/tge.in, and > config/gen/makefiles/root.in, pick a more meaningful variable name than > 'SHRPENV' and a more meaningful condition name than 'cygchkdll'. SHRPENV is directly from per5, and cygchkdll is they only way to check now for cygwin without the 57548-CONDITIONED_LINE_enh.patch. After that we can check against cygwin, the $^O. I really need some #+(and cygwin win32): sections. > Also in config/gen/makefiles/root.in we don't need a makefile target for > regenerating the makefile. Delete it. And you added a chunk of commented out > code again. Delete it. That was the framework to autodetect configuration changes, to get rid of make clean before perl Configure.pl. I do need to regenerate the makefile, that's why I added it for every makefile. I'll rather add the missing bits to the missings deps, than delete this logic. The commented-out section works in 90%, but has still a flaw somewhere, which could lead to cycles. I'll debug this when I have more time. > Since you've touched the config file and core PIR file for every language, > this branch will need extensive platform and language testing before it can > be merged in. I tested it extensively for the cygwin package. I believe I'm the only one who tested it at all because there were so many errors and still are. And I begin to wonder how others dare to release packages without working installables and with self.build make recipes. deb, gentoo, freebsd, rpm, the win32 build, and now my self-build work, but at least as Makefiles so we will have a standard. Just test-installable is a bit of work TODO. I had to test this by hand so far. > The branch is failing one test that passes in trunk, should be a quick fix: > t/codingstd/cuddled_else.t Thanks for reviewing! -- Reini Urban http://phpwiki.org/ http://murbreak.at/
