On Thu, Jun 26, 2014 at 2:54 PM, Carl Worth <cwo...@cworth.org> wrote:
> The preprocessor defines a notions of a "preprocessing number" that
> starts with either a digit or a decimal point, and continues with zero
> or more of digits, decimal points, identifier characters, or the sign
> symbols, ('-' and '+').
>
> Prior to this change, preprocessing numbers were lexed as some
> combination of OTHER and IDENTIFIER tokens. This had the problem of
> causing undesired macro expansion in some cases.
>
> We add tests to ensure that the undesired macro expansion does not
> happen in cases such as:
>
> #define e +1
> #define xyz -2
>
> int n = 1e;
> int p = 1xyz;
>
> In either case these macro definitions have no effect after this
> change, so that the numeric literals, (whether valid or not), will be
> passed on as-is from the preprocessor to the compiler proper.
>
> This fixes the following Khronos GLES3 CTS tests:
>
> preprocessor.basic.correct_phases_vertex
> preprocessor.basic.correct_phases_fragment
>
> v2. Thanks to Anuj Phogat for improving the original regular expression,
> (which accepted a '+' or '-', where these are only allowed after one of
> [eEpP]. I also expanded the test to exercise this.
>
> v3. Also fixed regular expression to require at least one digit at the
> beginning (after an optional period). Otherwise, a string such as ".xyz" was
> getting sucked up as a preprocessing number, (where obviously this should be a
> field access). Again, I expanded the test to exercise this.
>
V3 fixes a CTS failure caused by V2 in GL3Tests.shadow.shadow_compilation_frag.
> Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com>
> ---
>
> > It causes '2.0-MACRO' to incorrectly lexed as a preprocessing number.
> > Around 30 GLES3 CTS tests failed with this patch.
> >
> > I think using below definition of preprocessing numbers should work fine:
> > PP_NUMBER \.?[0-9]([._a-zA-Z0-9]|[eEpP][+-])*
>
> Thanks, Anuj. When I first saw your follow-up here, I saw the new
> alternation to pull the exponent piece, ("e+", etc.) separate from
> the main character class. But I missed that you als fixed the initial
> optional '.' followed by a required digit.
>
> I ended up implementing that second fix independently, and only now
> while sending the patch again do I see that you had that fixed in your
> regular expression as well.
>
> Thanks again. This new version also adds a bunch of testing to
> "make check", (and fixes all the regressions that piglit found with
> my earlier regular expression.
>
> src/glsl/glcpp/glcpp-lex.l | 6 ++++
> src/glsl/glcpp/tests/124-preprocessing-numbers.c | 37 +++++++++++++++++++++
> .../tests/124-preprocessing-numbers.c.expected | 38
> ++++++++++++++++++++++
> 3 files changed, 81 insertions(+)
> create mode 100644 src/glsl/glcpp/tests/124-preprocessing-numbers.c
> create mode 100644 src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
>
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index d5fb087..9563817 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -76,6 +76,7 @@ NEWLINE [\n]
> HSPACE [ \t]
> HASH ^{HSPACE}*#{HSPACE}*
> IDENTIFIER [_a-zA-Z][_a-zA-Z0-9]*
> +PP_NUMBER [.]?[0-9]([._a-zA-Z0-9]|[eEpP][-+])*
> PUNCTUATION [][(){}.&*~!/%<>^|;,=+-]
>
> /* The OTHER class is simply a catch-all for things that the CPP
> @@ -330,6 +331,11 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
> return IDENTIFIER;
> }
>
> +{PP_NUMBER} {
> + yylval->str = ralloc_strdup (yyextra, yytext);
> + return OTHER;
> +}
> +
> {PUNCTUATION} {
> return yytext[0];
> }
> diff --git a/src/glsl/glcpp/tests/124-preprocessing-numbers.c
> b/src/glsl/glcpp/tests/124-preprocessing-numbers.c
> new file mode 100644
> index 0000000..8019804
> --- /dev/null
> +++ b/src/glsl/glcpp/tests/124-preprocessing-numbers.c
> @@ -0,0 +1,37 @@
> +#define e THIS_SHOULD_NOT_BE_EXPANDED
> +#define E NOR_THIS
> +#define p NOT_THIS_EITHER
> +#define P AND_SURELY_NOT_THIS
> +#define OK CRAZY_BUT_TRUE_THIS_NEITHER
> +
> +/* This one is actually meant to be expanded */
> +#define MUST_EXPAND GO
> +
> +/* The following are "preprocessing numbers" and should not trigger macro
> + * expansion. */
> +1e
> +1OK
> +
> +/* These are also "preprocessing numbers", so no expansion */
> +123e+OK
> +.23E+OK
> +1.3e-OK
> +12.E-OK
> +123p+OK
> +.23P+OK
> +1.3p-OK
> +12.P-OK
> +123..OK
> +.23.OK.OK
> +
> +/* Importantly, just before the MUST_EXPAND in each of these, the preceding
> + * "preprocessing number" ends and we have an actual expression. So the
> + * MUST_EXPAND macro must be expanded (who would have though?) in each case.
> */
> +123ef+MUST_EXPAND
> +.23E3-MUST_EXPAND
> +1.3e--MUST_EXPAND
> +12.E-&MUST_EXPAND
> +123p+OK+MUST_EXPAND
> +.23P+OK;MUST_EXPAND
> +1.3p-OK-MUST_EXPAND
> +12.P-OK&MUST_EXPAND
> diff --git a/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
> b/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
> new file mode 100644
> index 0000000..6f5254c
> --- /dev/null
> +++ b/src/glsl/glcpp/tests/124-preprocessing-numbers.c.expected
> @@ -0,0 +1,38 @@
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +1e
> +1OK
> +
> +
> +123e+OK
> +.23E+OK
> +1.3e-OK
> +12.E-OK
> +123p+OK
> +.23P+OK
> +1.3p-OK
> +12.P-OK
> +123..OK
> +.23.OK.OK
> +
> +
> +
> +
> +123ef+GO
> +.23E3-GO
> +1.3e--GO
> +12.E-&GO
> +123p+OK+GO
> +.23P+OK;GO
> +1.3p-OK-GO
> +12.P-OK&GO
> +
Few trailing whitespaces in this file.
> --
> 2.0.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
