Kenneth Graunke <kenn...@whitecape.org> writes: > I agree that this is pretty bogus.
I'm coming around to thinking it's totally bogus.
> How about emitting a warning in the RETURN_TOKEN ('#') case?
Thanks for the review, and thanks for suggesting the warning. I added
the warning, then decided it needed some additional testing. So I
whipped up a test that uses every possible non-identifier character
to separate a macro name from its definition, like so:
#define S* splat
(defining a macro "S" to expand to "* splat"). This is first patch
attached below.
With this test, there's the question of whether it's even legal to have
no space between a macro name and the replacement list in a macro
definition. As usual, the GLSL specification is mute on the question,
(deferring just to "as is standard for C++"). If we choose GCC as the
standard, it does accept this case, (and emits a "missing space"
warning). By accident, glcpp has always silently accepted this, until
recently barfing, but only when '#' was the separating character.
My next patch expanded this new test so that '#' is tested as well, (and
ensuring the warning about the known-bogus '#' character was
emitted). This is the second patch attached below.
The expanded test looks really odd to me. Even though there are
something like two-dozen pieces of punctuation used a separators, only
the case with '#' emits a warning.
The insight is that the warning really has nothing to do with "#define
FOO*" but everything to do with the '#' character appearing out of
proper context. So what we really want is better testing of illegal
characters in various contexts.
So I wrote two additional new tests. The first runs every possible legal
character through glcpp, ensuring they are all passed through correctly.
It turns out this test caught a long-standing bug, (glcpp was tripping
up on vertical tab and form-feed for which the GLSL specification is
explicit should be allowed). The second test runs every possible illegal
character through glcpp, ensuring that an error is generated for each.
Instead of attaching those two tests, I'll send patches for them to the
list. They're independent of the current discussion.
And what was _really_ striking when looking at the final test (as I
first wrote it, but not as I will submit it), is that every illegal
character was generating an error message except for '#' which was just
generating a warning. So that sealed it for me that this treatment is
bogus.
So I'm inclined to leave Mesa breaking Reaction Quake, (but I'll
go file a bug against that application).
Now, what we could do if we were so inclined, would be to defer the
errors for illegal characters until they actually appeared in the
pre-processor output. That, is, a line such as:
$
would be an immediate error, ("Illegal character '$'), but a line such
as:
#define DOLLAR $
would be accepted with no error in and of itself, (the illegal character
has appeared in a replacement list, but may not ever appear in the
output to be seen by the compiler). Then, a subsequent line such as:
DOLLAR
would then emit the "Illegal character '$'" error.
If we had all of that in place, that would un-break Reaction Quake in a
way that wouldn't feel creepy to me, (and none of the tests would have
odd-looking exceptional behavior).
-Carl
From b57b0a5a88c318b71ac58a7c1569f07d416e6486 Mon Sep 17 00:00:00 2001
From: Carl Worth <cwo...@cworth.org>
Date: Tue, 5 Aug 2014 11:31:56 -0700
Subject: [PATCH] glsl/glcpp: Add testing for no space between macro name and
replacement list
GCC's preprocessor accepts a macro definition where there is no space between
the macro's identifier name and the replacementlist. (GCC does emit a "missing
space" warning that we don't, but that's fine.)
This is an exhaustive test that verifies that all legal GLSL characters that
could possibly be interpreted as separating the macro name from the
replacement list are interpreted as such. So the testing here includes all
valid GLSL symbols except for:
* Characters that can be part of an identifier (a-z, A-Z, 0-9, _)
* Backslash, (allowed only as line continuation)
* Hash, (allowed only to introduce pre-processor directive, or as part of a
paste operator in a replacement list---but not as first token of
replacement list)
* Space characters (since the point of the testing is to have missing space)
* Left parenthesis (which would indicate a function-like macro)
---
src/glsl/glcpp/tests/139-define-macro-no-space.c | 52 ++++++++++++++++++++++
.../tests/139-define-macro-no-space.c.expected | 52 ++++++++++++++++++++++
2 files changed, 104 insertions(+)
create mode 100644 src/glsl/glcpp/tests/139-define-macro-no-space.c
create mode 100644 src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
diff --git a/src/glsl/glcpp/tests/139-define-macro-no-space.c b/src/glsl/glcpp/tests/139-define-macro-no-space.c
new file mode 100644
index 0000000..4e997a0
--- /dev/null
+++ b/src/glsl/glcpp/tests/139-define-macro-no-space.c
@@ -0,0 +1,52 @@
+/* The GLSL specification is not specific about how to handle a non-space
+ * character separating a macro identifier from the replacement list. It says
+ * only "as is standard for C++ preprocessors". GCC accepts these and warns of
+ * "missing whitespace". So we'll accept these, (though we don't warn).
+ */
+#define A& ampersand
+#define B! bang
+#define C, comma
+#define D/ divider
+#define E= equals
+#define F. full stop
+#define G> greater than
+#define H- hyphen
+#define I+ incrementor
+#define J[ JSON array
+#define K} kurly brace?
+#define L< less than
+#define M{ moustache
+#define N^ nose
+#define P) parenthesis (right)
+#define Q? question mark
+#define R% ratio indicator
+#define S] square bracket (right)
+#define T~ tilde
+#define U: umlaut?
+#define V| vertical bar
+#define W; wink
+#define X* X (as multiplication)
+A
+B
+C
+D
+E
+F
+G
+H
+I
+J
+K
+L
+M
+N
+P
+Q
+R
+S
+T
+U
+V
+W
+X
+
diff --git a/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected b/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
new file mode 100644
index 0000000..665d44a
--- /dev/null
+++ b/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
@@ -0,0 +1,52 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+& ampersand
+! bang
+, comma
+/ divider
+= equals
+. full stop
+> greater than
+- hyphen
++ incrementor
+[ JSON array
+} kurly brace?
+< less than
+{ moustache
+^ nose
+) parenthesis (right)
+? question mark
+% ratio indicator
+] square bracket (right)
+~ tilde
+: umlaut?
+| vertical bar
+; wink
+* X (as multiplication)
+
--
2.0.0
From 5443b2f628f25af25605eb6793dd036596a33380 Mon Sep 17 00:00:00 2001
From: Carl Worth <cwo...@cworth.org>
Date: Tue, 5 Aug 2014 12:06:59 -0700
Subject: [PATCH] glsl/glcpp: Also test '#' as separator of macro identifier
and definition
Strictly speaking, in GLSL, the '#' symbol should never appear in a macro
definition, (the "replacement list"), except as part of a paste operator. But
even the paste operator is illegal at the beginning of the replacement list.
So the '#' symbol should never appear to separate the identifier from the
replacement list. We allow this only because we saw this accidentally occur
in an application (it wasn't intentional in that application, but it wasn't
harmful either).
So we accept this, but we do warn about it.
---
src/glsl/glcpp/tests/139-define-macro-no-space.c | 2 ++
src/glsl/glcpp/tests/139-define-macro-no-space.c.expected | 3 +++
2 files changed, 5 insertions(+)
diff --git a/src/glsl/glcpp/tests/139-define-macro-no-space.c b/src/glsl/glcpp/tests/139-define-macro-no-space.c
index 4e997a0..04e19fe 100644
--- a/src/glsl/glcpp/tests/139-define-macro-no-space.c
+++ b/src/glsl/glcpp/tests/139-define-macro-no-space.c
@@ -17,6 +17,7 @@
#define L< less than
#define M{ moustache
#define N^ nose
+#define O# octothorpe
#define P) parenthesis (right)
#define Q? question mark
#define R% ratio indicator
@@ -40,6 +41,7 @@ K
L
M
N
+O
P
Q
R
diff --git a/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected b/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
index 665d44a..2c30c25 100644
--- a/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
+++ b/src/glsl/glcpp/tests/139-define-macro-no-space.c.expected
@@ -1,3 +1,4 @@
+0:20(10): preprocessor warning: Unexpected symbol '#' (not a preprocessing directive)
@@ -26,6 +27,7 @@
+
& ampersand
! bang
, comma
@@ -40,6 +42,7 @@
< less than
{ moustache
^ nose
+# octothorpe
) parenthesis (right)
? question mark
% ratio indicator
--
2.0.0
pgp1AGrTcuI1E.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
