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.
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.)
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).
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))". 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.
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'.
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.
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.
The branch is failing one test that passes in trunk, should be a quick fix:
t/codingstd/cuddled_else.t
Thanks!
Allison
