Front page | perl.perl5.porters |
Postings from April 2013
On 04/29/2013 08:49 AM, Nicholas Clark wrote: > On Sun, Apr 28, 2013 at 02:39:52PM -0700, Father Chrysostomos via RT wrote: >> On Tue Apr 23 13:09:41 2013, khw wrote: >>> Attached is a patch for review that fails with a more appropriate error >>> message. As you can see in the commit message, the scope of the problem >>> is somewhat larger than previously thought. >>> >> >> Thank you. >> >> If you are fixing all the (?...) constructs at the same time (which I >> think you are), then I am all for this patch. >> >> What makes me uncomfortable is leaving Perl in an inconsistent state in >> which *some* (?...) constructs allow the space and some do not. > > I agree - it's better to be consistent, even if it's a consistently ugly > state. (and the same ugly as the previous releases.) > > I'm not at all familiar with that code, so I can't say with any confidence > whether this change gets all of them, or leaves some uncovered. I hope that > someone else is confident to assess this. > > Nicholas Clark > I am confident that it catches all of them, and also (*VERB...) constructs, which it turns out have the same issue; otherwise I wouldn't have submitted the patch :) I'm also confident that this patch only affects the intended things. And I can make a detailed case for that if necessary. And I am confident that something like this patch is the right thing to do going forward. That a (?#...) comment currently can split the (? and (* tokens without having to have /x makes this more clear. What I am unsure of is if we should, so close to 5.18, be making something a syntax error that previously wasn't. There is one place in our code, utf8_heavy.pl, which has '( ?', and must be corrected before this patch is applied. I wrote that offending code, so I know that the intervening space was just an unintended typo that never got noticed. Even making this a deprecation warning instead of a syntax error can cause code to fail. If we don't do anything, 5.18 will have a regression misleading error message. If we take this patch, but make it a warning instead of an error, 5.18 will have the misleading error, plus this warning ahead of it. I suppose we could tweak the patch so that it gives an error in precisely the same places as the misleading error would, and lets all the other problematic constructs go by, unwarned on, and then change it in 5.19 to warn. I haven't investigated how hard this would be. The other alternative I can think of is to revert the patch that caused this regression. I haven't investigated enough to understand the implications of that.Thread Previous | Thread Next