develooper Front page | perl.perl5.porters | Postings from April 2013

Re: [perl #117327] Sequence (?#...) not recognized in regex

Thread Previous | Thread Next
Karl Williamson
April 29, 2013 18:13
Re: [perl #117327] Sequence (?#...) not recognized in regex
Message ID:
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,, 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 Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About