On 10/29/24 11:31 AM, Jakub Jelinek wrote:
On Fri, Oct 25, 2024 at 12:52:41PM -0400, Jason Merrill wrote:
This does seem like a hole in the wording. I think the clear intent is that
the name/partition must neither be macro-expanded nor come from macro
expansion.
I'll defer filing the DR and figuring out the right wording for the standard
to you/WG21.
I've reported it to CWG, and people agree with both my understanding and
the need for wording improvement.
The patch below implements what is above described as the first variant
of the first issue resolution, i.e. disables expansion of as many tokens
as could be in the valid module name and module partition syntax, but
as soon as it e.g. sees two adjacent identifiers, the second one can be
macro expanded. So, effectively:
#define SEMI ;
export module SEMI
used to be valid and isn't anymore,
#define FOO bar
export module FOO;
isn't valid,
#define COLON :
export module COLON private;
isn't valid,
#define BAR baz
export module foo.bar:baz.qux.BAR;
isn't valid,
Agreed.
Ok.
but
#define BAZ .qux
export module foo BAZ;
or
#define QUX [[]]
export module foo QUX;
or
#define FREDDY :garply
export module foo FREDDY;
or
#define GARPLY private
module : GARPLY;
etc. is.
I think QUX is valid, but the others are intended to be invalid.
I've changed the patch so that the BAZ and FREDDY cases above
are also diagnosed as invalid, so cases where a module name or module
partition name is followed by an identifier defined as object-like or
function-like macro and where after macro expansion the first
non-padding/comment token is dot or colon (so, either an object-like
or function-like macro whose expansion starts with . or : or one
which expands to nothing and . or : following that macro either directly
or from some other macro expansion.
E.g. one could have
#define A [[vendor::attr(1 ? 2
#define B 3)]]
export module foo.bar A : B;
which I think ought to be valid and expanded as
export module foo.bar [[vendor::attr(1 ? 2 : 3)]];
or
#define A
#define B baz
export module foo.bar A : B;
which would preprocess into
export module foo.bar:baz;
and I think that is against the intent of the paper where the quick
scanners couldn't figure this out without actually performing preprocessing.
Unfortunately, trying to enter macro context inside of
cpp_maybe_module_directive doesn't seem to work well, so I've instead added
a flag and let cpp_get_token_1 to diagnose it later (but still during
preprocessing, because if going e.g. with -save-temps the distinction would
be lost, so it feels undesirable to diagnose that during parsing, plus
whether some token comes from a macro or not is currently only testable in
the FE when not -fno-track-macro-expansion and see above testcase that
the : or . actually might not come from a macro, yet there could be
a macro expanding to nothing in between.
Sounds good.
I've kept the GARPLY case above as valid, I don't see the paper changing
anything in that regard (sure, the : has to not come from macro so that
the scanners can figure it out) and I think it isn't needed.
It seems to me that no macro expansion should be done for a directive
starting "module :" as that cannot be a pp-module.
--- gcc/testsuite/g++.dg/modules/dir-only-4.C.jj 2020-12-22
23:50:17.057972516 +0100
+++ gcc/testsuite/g++.dg/modules/dir-only-4.C 2024-08-08 09:33:09.454522024
+0200
@@ -1,8 +1,8 @@
// { dg-additional-options {-fmodules-ts -fpreprocessed -fdirectives-only} }
-// { dg-module-cmi !foo }
+// { dg-module-cmi !baz }
module;
#define foo baz
-export module foo;
+export module baz;
class import {};
--- gcc/testsuite/g++.dg/modules/atom-preamble-2_a.C.jj 2020-12-22
23:50:17.055972539 +0100
+++ gcc/testsuite/g++.dg/modules/atom-preamble-2_a.C 2024-08-08
09:35:56.093364042 +0200
@@ -1,6 +1,6 @@
// { dg-additional-options "-fmodules-ts" }
#define malcolm kevin
-export module malcolm;
+export module kevin;
// { dg-module-cmi kevin }
export class X;
--- gcc/testsuite/g++.dg/modules/atom-preamble-4.C.jj 2020-12-22
23:50:17.055972539 +0100
+++ gcc/testsuite/g++.dg/modules/atom-preamble-4.C 2024-08-08
09:35:32.463670046 +0200
@@ -1,5 +1,5 @@
// { dg-additional-options "-fmodules-ts" }
-#define NAME(X) X;
+#define NAME(X) ;
-export module NAME(bob)
+export module bob NAME(bob)
It looks like these tests were specifically testing patterns that this paper
makes ill-formed, and so we should check for errors instead of changing them
to be well-formed.
Ok, changed dir-only-4.C and atom-preamble-4.C back to previous content
+ dg-error and added new tests with the changes in and not expecting
error.
I had to keep the atom-preamble-2_a.C change as is, because otherwise
all of atom-preamble-2*.C tests FAIL.
Fair enough. But then let's also remove the #define that is no longer used.
Incidentally, I think 2_b.C should fail because the pp-module is in the
midle of an if-section, which is forbidden by the [cpp] grammar since P1857.
Here is a so far lightly tested patch, ok for trunk if it passes full
bootstrap/regtest or do you want some further changes?
OK with removing the #define from atom-preamble-2_a.C.
Jason
