Incorporate code review feedback for (?[])
authorKarl Williamson <public@khwilliamson.com>
Wed, 30 Jan 2013 19:39:51 +0000 (12:39 -0700)
committerKarl Williamson <public@khwilliamson.com>
Mon, 4 Feb 2013 04:41:28 +0000 (21:41 -0700)
Thanks to Hugo van der Sanden for reviewing this new code.

embed.fnc
embed.h
pod/perlre.pod
proto.h
regcomp.c

index fee4826..cb8e5e8 100644 (file)
--- a/embed.fnc
+++ b/embed.fnc
@@ -1977,9 +1977,9 @@ Es        |regnode*|regclass      |NN struct RExC_state_t *pRExC_state \
                                |bool allow_multi_fold                        \
                                |const bool silence_non_portable              \
                                |NULLOK SV** ret_invlist
-Es     |bool|could_it_be_POSIX |NN struct RExC_state_t *pRExC_state
-Es     |regnode*|handle_sets   |NN struct RExC_state_t *pRExC_state \
-                               |NN I32 *flagp|U32 depth \
+Es     |bool|could_it_be_a_POSIX_class|NN struct RExC_state_t *pRExC_state
+Es     |regnode*|handle_regex_sets|NN struct RExC_state_t *pRExC_state \
+                               |NN I32 *flagp|U32 depth                \
                                |NN char * const oregcomp_parse
 Es     |void|parse_lparen_question_flags|NN struct RExC_state_t *pRExC_state
 Es     |regnode*|reg_node      |NN struct RExC_state_t *pRExC_state|U8 op
diff --git a/embed.h b/embed.h
index 592aa73..e9ff0e8 100644 (file)
--- a/embed.h
+++ b/embed.h
 #define cl_is_anything         S_cl_is_anything
 #define cl_or                  S_cl_or
 #define compute_EXACTish(a)    S_compute_EXACTish(aTHX_ a)
-#define could_it_be_POSIX(a)   S_could_it_be_POSIX(aTHX_ a)
+#define could_it_be_a_POSIX_class(a)   S_could_it_be_a_POSIX_class(aTHX_ a)
 #define get_invlist_iter_addr(a)       S_get_invlist_iter_addr(aTHX_ a)
 #define get_invlist_previous_index_addr(a)     S_get_invlist_previous_index_addr(aTHX_ a)
 #define get_invlist_version_id_addr(a) S_get_invlist_version_id_addr(aTHX_ a)
 #define get_invlist_zero_addr(a)       S_get_invlist_zero_addr(aTHX_ a)
 #define grok_bslash_N(a,b,c,d,e,f,g)   S_grok_bslash_N(aTHX_ a,b,c,d,e,f,g)
-#define handle_sets(a,b,c,d)   S_handle_sets(aTHX_ a,b,c,d)
+#define handle_regex_sets(a,b,c,d)     S_handle_regex_sets(aTHX_ a,b,c,d)
 #define invlist_array(a)       S_invlist_array(aTHX_ a)
 #define invlist_clone(a)       S_invlist_clone(aTHX_ a)
 #define invlist_extend(a,b)    S_invlist_extend(aTHX_ a,b)
index 98fd36e..414ba38 100644 (file)
@@ -1736,15 +1736,6 @@ to inside of one of these constructs. The following equivalences apply:
 =item C<(?[ ])>
 X<set operations>
 
-This is an experimental feature present starting in 5.18, but is subject
-to change as we gain field experience with it.  Any attempt to use it
-will raise a warning, unless disabled via
-
- no warnings "experimental::regex_sets";
-
-Comments on this feature are welcome; send email to
-C<perl5-porters@perl.org>.
-
 This is a fancy bracketed character class that can be used for more
 readable and less error-prone classes, and to perform set operations,
 such as intersection. An example is
@@ -1752,7 +1743,17 @@ such as intersection. An example is
  /(?[ \p{Thai} & \p{Digit} ])/
 
 This will match all the digit characters that are in the Thai script.
-We can extend this by
+
+This is an experimental feature available starting in 5.18, but is
+subject to change as we gain field experience with it.  Any attempt to
+use it will raise a warning, unless disabled via
+
+ no warnings "experimental::regex_sets";
+
+Comments on this feature are welcome; send email to
+C<perl5-porters@perl.org>.
+
+We can extend the example above:
 
  /(?[ ( \p{Thai} + \p{Lao} ) & \p{Digit} ])/
 
@@ -1780,7 +1781,12 @@ There is one unary operator:
 
 All the binary operators left associate, and are of equal precedence.
 The unary operator right associates, and has higher precedence.  Use
-parentheses to override the default associations.
+parentheses to override the default associations.  Some feedback we've
+received indicates a desire for intersection to have higher precedence
+than union.  This is something that feedback from the field may cause us
+to change in future releases; you may want to parenthesize copiously to
+avoid such changes affecting your code, until this feature is no longer
+considered experimental.
 
 The main restriction is that everything is a metacharacter.  Thus,
 you cannot refer to single characters by doing something like this:
@@ -1793,7 +1799,7 @@ it in brackets:
  /(?[ [a] + [b] ])/
 
 (This is the same thing as C<[ab]>.)  You could also have said the
-equivalent
+equivalent:
 
  /(?[[ a b ]])/
 
@@ -1801,16 +1807,21 @@ equivalent
 C<\N{ }>, etc.)
 
 This last example shows the use of this construct to specify an ordinary
-bracketed character class without set operations.  Note the white space
-within it.  To specify a matchable white space character, you can escape
-it with a backslash, like:
+bracketed character class without additional set operations.  Note the
+white space within it; L</E<sol>x> is turned on even within bracketed
+character classes, except you can't have comments inside them.  Hence,
+
+ (?[ [#] ])
+
+matches the literal character "#".  To specify a literal white space character,
+you can escape it with a backslash, like:
 
  /(?[ [ a e i o u \  ] ])/
 
 This matches the English vowels plus the SPACE character.
 All the other escapes accepted by normal bracketed character classes are
-accepted here as well; but unlike the normal ones, unrecognized escapes are
-fatal errors here.
+accepted here as well; but unrecognized escapes that generate warnings
+in normal classes are fatal errors here.
 
 All warnings from these class elements are fatal, as well as some
 practices that don't currently warn.  For example you cannot say
@@ -1822,28 +1833,50 @@ zero to make two).  These restrictions are to lower the incidence of
 typos causing the class to not match what you thought it would.
 
 The final difference between regular bracketed character classes and
-these, is that it is not possible to get the latter to match a
+these, is that it is not possible to get these to match a
 multi-character fold.  Thus,
 
  /(?[ [\xDF] ])/iu
 
 does not match the string C<ss>.
 
-You don't have to enclose Posix class names inside double brackets.  The
-following works
+You don't have to enclose Posix class names inside double brackets,
+hence both of the following work:
 
  /(?[ [:word:] - [:lower:] ])/
+ /(?[ [[:word:]] - [[:lower:]] ])/
 
-C<< (?[ ]) >> is a compile-time construct.  Any attempt to use something
-which isn't knowable until run-time is a fatal error.  Thus, this
-construct cannot be used within the scope of C<use locale> (or the
-L</C<E<sol>l>> regex modifier).  Any L<user-defined
-property|perlunicode/"User-Defined Character Properties"> used must be
-already defined by the time the regular expression is compiled; but note
-that this construct can be used to avoid defining such properties.
+The Posix character classes, including things like C<\w> and C<\D>
+respect the L</E<sol>a (and E<sol>aa)> modifiers.
 
-A regular expression using this construct that otherwise would compile
-using L</C<E<sol>d>> rules will instead use L</C<E<sol>u>>.
+C<< (?[ ]) >> is a regex-compile-time construct.  Any attempt to use
+something which isn't knowable at the time the containing regular
+expression is compiled is a fatal error.  In practice, this means
+just three limitiations:
+
+=over 4
+
+=item 1
+
+This construct cannot be used within the scope of
+C<use locale> (or the L</C<E<sol>l>> regex modifier).
+
+=item 2
+
+Any
+L<user-defined property|perlunicode/"User-Defined Character Properties">
+used must be already defined by the time the regular expression is
+compiled (but note that this construct can be used instead of such
+properties).
+
+=item 3
+
+A regular expression that otherwise would compile
+using L</C<E<sol>d>> rules, and which uses this construct will instead
+use L</C<E<sol>u>>.  Thus this construct tells Perl that you don't want
+L</E<sol>d> rules for the entire regular expression containing it.
+
+=back
 
 The L</C<E<sol>x>> processing within this class is an extended form.
 Besides the characters that are considered white space in normal C</x>
@@ -1860,13 +1893,17 @@ construct.  There must not be any space between any of the characters
 that form the initial C<(?[>.  Nor may there be space between the
 closing C<])> characters.
 
-Due to the way that Perl parses things, your parentheses and brackets
-may need to be balanced, even including comments.
 
-Since this experimental, we may change this so that other legal uses of
-normal bracketed character classes might become illegal.  One proposal,
-for example, is to forbid adjacent uses of the same character, as in
-C<[aa]>.  This is likely a typo, as the second "a" adds nothing.
+Due to the way that Perl parses things, your parentheses and brackets
+may need to be balanced, even including comments.  If you run into any
+examples, please send them to C<perlbug@perl.org>, so that we can have a
+concrete example for this man page.
+
+We may change it so that things that remain legal uses in normal bracketed
+character classes might become illegal within this experimental
+construct.  One proposal, for example, is to forbid adjacent uses of the
+same character, as in C<(?[ [aa] ])>.  The motivation for such a change
+is that this usage is likely a typo, as the second "a" adds nothing.
 
 =back
 
diff --git a/proto.h b/proto.h
index fde67d9..40f2953 100644 (file)
--- a/proto.h
+++ b/proto.h
@@ -6481,9 +6481,9 @@ PERL_STATIC_INLINE U8     S_compute_EXACTish(pTHX_ struct RExC_state_t *pRExC_state)
 #define PERL_ARGS_ASSERT_COMPUTE_EXACTISH      \
        assert(pRExC_state)
 
-STATIC bool    S_could_it_be_POSIX(pTHX_ struct RExC_state_t *pRExC_state)
+STATIC bool    S_could_it_be_a_POSIX_class(pTHX_ struct RExC_state_t *pRExC_state)
                        __attribute__nonnull__(pTHX_1);
-#define PERL_ARGS_ASSERT_COULD_IT_BE_POSIX     \
+#define PERL_ARGS_ASSERT_COULD_IT_BE_A_POSIX_CLASS     \
        assert(pRExC_state)
 
 PERL_STATIC_INLINE UV* S_get_invlist_iter_addr(pTHX_ SV* invlist)
@@ -6516,11 +6516,11 @@ STATIC bool     S_grok_bslash_N(pTHX_ struct RExC_state_t *pRExC_state, regnode** no
 #define PERL_ARGS_ASSERT_GROK_BSLASH_N \
        assert(pRExC_state); assert(flagp)
 
-STATIC regnode*        S_handle_sets(pTHX_ struct RExC_state_t *pRExC_state, I32 *flagp, U32 depth, char * const oregcomp_parse)
+STATIC regnode*        S_handle_regex_sets(pTHX_ struct RExC_state_t *pRExC_state, I32 *flagp, U32 depth, char * const oregcomp_parse)
                        __attribute__nonnull__(pTHX_1)
                        __attribute__nonnull__(pTHX_2)
                        __attribute__nonnull__(pTHX_4);
-#define PERL_ARGS_ASSERT_HANDLE_SETS   \
+#define PERL_ARGS_ASSERT_HANDLE_REGEX_SETS     \
        assert(pRExC_state); assert(flagp); assert(oregcomp_parse)
 
 PERL_STATIC_INLINE UV* S_invlist_array(pTHX_ SV* const invlist)
index 8e9fa31..53589ad 100644 (file)
--- a/regcomp.c
+++ b/regcomp.c
@@ -9110,7 +9110,8 @@ S_reg(pTHX_ RExC_state_t *pRExC_state, I32 paren, I32 *flagp,U32 depth)
                }
            }
            case '[':           /* (?[ ... ]) */
-                return handle_sets(pRExC_state, flagp, depth, oregcomp_parse);
+                return handle_regex_sets(pRExC_state, flagp, depth,
+                                         oregcomp_parse);
             case 0:
                RExC_parse--; /* for vFAIL to print correctly */
                 vFAIL("Sequence (? incomplete");
@@ -11363,7 +11364,7 @@ S_regpposixcc(pTHX_ RExC_state_t *pRExC_state, I32 value, SV *free_me,
 }
 
 STATIC bool
-S_could_it_be_POSIX(pTHX_ RExC_state_t *pRExC_state)
+S_could_it_be_a_POSIX_class(pTHX_ RExC_state_t *pRExC_state)
 {
     /* This applies some heuristics at the current parse position (which should
      * be at a '[') to see if what follows might be intended to be a [:posix:]
@@ -11377,7 +11378,7 @@ S_could_it_be_POSIX(pTHX_ RExC_state_t *pRExC_state)
      *                         ')' indicating the end of the (?[
      *      [:any garbage including %^&$ punctuation:]
      *
-     * This is designed to be called only from S_handle_sets; it could be
+     * This is designed to be called only from S_handle_regex_sets; it could be
      * easily adapted to be called from the spot at the beginning of regclass()
      * that checks to see in a normal bracketed class if the surrounding []
      * have been omitted ([:word:] instead of [[:word:]]).  But doing so would
@@ -11385,7 +11386,7 @@ S_could_it_be_POSIX(pTHX_ RExC_state_t *pRExC_state)
     char* p = RExC_parse + 1;
     char first_char = *p;
 
-    PERL_ARGS_ASSERT_COULD_IT_BE_POSIX;
+    PERL_ARGS_ASSERT_COULD_IT_BE_A_POSIX_CLASS;
 
     assert(*(p - 1) == '[');
 
@@ -11416,7 +11417,7 @@ S_could_it_be_POSIX(pTHX_ RExC_state_t *pRExC_state)
 }
 
 STATIC regnode *
-S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
+S_handle_regex_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
                    char * const oregcomp_parse)
 {
     /* Handle the (?[...]) construct to do set operations */
@@ -11433,7 +11434,7 @@ S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
 
     GET_RE_DEBUG_FLAGS_DECL;
 
-    PERL_ARGS_ASSERT_HANDLE_SETS;
+    PERL_ARGS_ASSERT_HANDLE_REGEX_SETS;
 
     if (LOC) {
         vFAIL("(?[...]) not valid in locale");
@@ -11462,19 +11463,23 @@ S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
                 default:
                     break;
                 case '\\':
-                    /* Skip the next byte.  This would have to change to skip
-                     * the next character if we were to recognize and handle
-                     * specific non-ASCIIs */
+                    /* Skip the next byte (which could cause us to end up in
+                     * the middle of a UTF-8 character, but since none of those
+                     * are confusable with anything we currently handle in this
+                     * switch (invariants all), it's safe.  We'll just hit the
+                     * default: case next time and keep on incrementing until
+                     * we find one of the invariants we do handle. */
                     RExC_parse++;
                     break;
                 case '[':
                 {
                     /* If this looks like it is a [:posix:] class, leave the
                      * parse pointer at the '[' to fool regclass() into
-                     * thinking it is part of a '[[:posix]]'.  That function
+                     * thinking it is part of a '[[:posix:]]'.  That function
                      * will use strict checking to force a syntax error if it
                      * doesn't work out to a legitimate class */
-                    bool is_posix_class = could_it_be_POSIX(pRExC_state);
+                    bool is_posix_class
+                                    = could_it_be_a_POSIX_class(pRExC_state);
                     if (! is_posix_class) {
                         RExC_parse++;
                     }
@@ -11546,11 +11551,11 @@ S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
      * above.
      *
      * A '(' is simply pushed on the stack; it is valid only if the stack is
-     * empty, or the top element of the stack is an operator (for which the
-     * parenthesized expression will become an operand).  By the time the
-     * corresponding ')' is parsed everything in between should have been
-     * parsed and evaluated to a single operand (or else is a syntax error),
-     * and is handled as a regular operand */
+     * empty, or the top element of the stack is an operator or another '('
+     * (for which the parenthesized expression will become an operand).  By the
+     * time the corresponding ')' is parsed everything in between should have
+     * been parsed and evaluated to a single operand (or else is a syntax
+     * error), and is handled as a regular operand */
 
     stack = newAV();
 
@@ -11562,9 +11567,10 @@ S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
         /* Skip white space */
         RExC_parse = regpatws(pRExC_state, RExC_parse,
                                 TRUE); /* means recognize comments */
-        if (RExC_parse >= RExC_end
-            || (curchar = UCHARAT(RExC_parse)) == ']')
-        {   /* Exit loop at the end */
+        if (RExC_parse >= RExC_end) {
+            Perl_croak(aTHX_ "panic: Read past end of '(?[ ])'");
+        }
+        if ((curchar = UCHARAT(RExC_parse)) == ']') {
             break;
         }
 
@@ -11588,7 +11594,7 @@ S_handle_sets(pTHX_ RExC_state_t *pRExC_state, I32 *flagp, U32 depth,
 
             case '[':   /* Is a bracketed character class */
             {
-                bool is_posix_class = could_it_be_POSIX(pRExC_state);
+                bool is_posix_class = could_it_be_a_POSIX_class(pRExC_state);
 
                 if (! is_posix_class) {
                     RExC_parse++;