Hi Stuart!
Thanks for the comments, and please see my answers inline:
On 9/5/19 1:18 PM, Stuart Marks wrote:
Conceptually I think having the restriction is fine. If we were
designing this as a new feature, I'd recommend putting in the
restrictions from the very beginning.
However, since the old behavior has been out there for 20 years, my
main concern is compatibility. Having system properties to control
this is ... ok ... I guess. I wish there were a better way, but it
seems the simplest. However, I'd strongly advise making the behavior
change and the compatibility mode(s) as simple as possible. Having
more configuration options complicates the code and complicates the
testing, and I really don't see the need for it.
So yes, I agree with abandoning ALLOW_LOWERCASE_CONTROL_CHAR_IDS.
Okay. I've just sent out the updated webrev without this option:
http://cr.openjdk.java.net/~igerasim/8230365/03/webrev/
And filed the CSR for the spec change and the new property:
https://bugs.openjdk.java.net/browse/JDK-8230675
I guess now we get to have a bikeshed on the property name. Does this
apply only to mapping of control characters, or is there some other
form of pattern syntax that could be made more strict? Ivan, I seem to
recall you talking to me about making some changes in the syntax or
interpretation of the construct that enables/disables various flags.
This one:
|(?idmsuxU-idmsuxU)|
Would it make sense to have a single property, e.g.
jdk.util.regex.strictSyntax, that governs this one as well? And are
there other possibilities for tuning up the parsing behavior that we
should be taking?
Yes, that was this bug:
https://bugs.openjdk.java.net/browse/JDK-8225021 (Treat ambiguous
embedded flags as parse syntax errors)
And actually I have yet another bug of similar flavor:
https://bugs.openjdk.java.net/browse/JDK-8214245 (Case insensitive
matching doesn't work correctly for some character classes)
However, this last one is about changing the matching rules, and not
about the syntax (And it is still waiting for review!).
I'd rather not have a situation where we fix up one area of syntax and
add a property for it, fix up another area and add a different
property, leading to property proliferation.
Personally, I don't have a strong preference here.
The compatibility property are meant to be temporary anyways.
Maybe if we have a single option that will control several different
aspects of behavior, it will be harder to get rid of it?
Partially, because it will be tempting to reuse it for other similar
changes, should they be needed.
With kind regards,
Ivan
s'marks
On 9/4/19 9:00 PM, Martin Buchholz wrote:
Thanks, Ivan. We're mostly in agreement.
+ * If {@code true} then lower-case control-character ids are mapped to the
+ * their upper-case counterparts.
Extra "the".
After all these decades I only now realize that c ^= 0x40 moves '?'
to the end of the ASCII range and all the other controls to the start!
Should we support lower-case controls? Compatibility with perl regex
still matters, but a lot less than in 2003. But the key is that we
got the WRONG ANSWER previously, so when we restrict the control ids
let's just make lower case controls syntax errors. Silently changing
behavior is bad for users. ... so let's abandon
ALLOW_LOWERCASE_CONTROL_CHAR_IDS.
An alternative:
int ch = read() ^ 0x40;
if (!RESTRICTED_CONTROL_CHAR_IDS || ch < 0x20 || ch == 0x7f) return ch;
On Wed, Sep 4, 2019 at 6:49 PM Ivan Gerasimov
<ivan.gerasi...@oracle.com <mailto:ivan.gerasi...@oracle.com>> wrote:
Thank you Martin!
On 8/30/19 6:19 PM, Martin Buchholz wrote:
> There's a strong expectation that ctrl-A and ctrl-a both map to
> \u0001, so I support Ivan's initiative.
> I'm surprised java regex gets this wrong.
> Might need a transitional system property.
>
Right. I think it would be best to introduce two system properties:
The first, to turn on/off the restrictions on the control-char
names.
This will be enabled by default, and will permit names from the
limited
list: capital letters and a few other special characters.
The second one, to enable mapping of lower-case control-char
names to
their upper-case counterpart. This option should be turned off by
default for the current release of JDK, and then turned on by
default
for some subsequent release (when, presumably, most applications
that
use this kind of regexp are fixed).
This all feels like a little bit too much for such a rarely used
feature, but probably is a proper thing to do anyway :-)
If we have an agreement on these system properties, I can create a
separate test to verify all possible combinations.
> What's the best bit-twiddle? Untested:
> if ((c ^= 0x40) < 0x20) return c;
> if ((c ^=0x20) <= 26 && c > 0) return c;
>
> 0x40 is more readable than 64.
>
`((ch-0x3f)|(0x5f-ch)) >= 0` does the trick for regular
(non-lower-case)
ids.
> Does ctrol-? get mapped to 0x7f ?
>
Yes. I've got it in the test at the end of the line 4997.
Would you please help review the updated webrev:
http://cr.openjdk.java.net/~igerasim/8230365/02/webrev/
With kind regards,
Ivan
--
With kind regards,
Ivan Gerasimov
