[PATCH] D122885: [clang] Draft: Implement P1703R1

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

oh, also in 'import "bob";', "bob" needs to be lexed as a #include name, not a string.  (and likewise 'import <bob>;')  Hence treating these lines in the preprocessor in directive-processing-mode was the way GCC went.



================
Comment at: clang/test/Modules/P1703R1.cpp:49-50
+"header.h"; // expected-error {{expected a module name after 'import'}}
+export
+import "header.h"; // expected-error {{expected a module name after 'import'}}
+
----------------
This confuses me.  That's a stray 'export' token, followed by a well-formed import-directive


================
Comment at: clang/test/Modules/P1703R1.cpp:52
+
+import "header.h"; import "header.h"; // expected-error {{import keyword must be at start of line}}
+                                      // expected-error at -1 {{extra tokens after module import}}
----------------
that second 'import' is not a token, it's an identifier.  This is an ill-formed import-directive with unexpected tokens afte the first ';'  You seem to be dropping into lex-import-mode not at the start of a line?


================
Comment at: clang/test/Modules/P1703R1.cpp:72
+
+/// Normal module imports are unaffected
+import dummy;
----------------
what is meant by 'normal'?  many of these look ill-formed to me.


================
Comment at: clang/test/Modules/P1703R1.cpp:87
+
+// TODO: The diagnostics here could be much better.
+export
----------------
yup :)  gcc tries to tell the user about the single logical-line requirement:
   inform (token->location, "perhaps insert a line break, or other"
	      " disambiguation, to prevent this being considered a"
	      " module control-line");
the expectation here is that this was pre c++20 code that fell into this lexing trap.  Specifically things like:

template<typename T> class module {...};

module<mytype> x;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122885/new/

https://reviews.llvm.org/D122885