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

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan requested changes to this revision.
urnathan added a comment.
This revision now requires changes to proceed.

Either update this when you're ready, or abandon and create a new one for 1857.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

FWIW probably these 5 patches are pretty independent of changes to the 
preprocessor...
...  the first four of those are extremely unlikely to clash - since they are 
concerned with the driver.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D122885#3431618 , @tbaeder wrote:

> What patches are you talking about exactly?

I mean D121588  D121589 
 D121590  
D121591  D122119 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

What patches are you talking about exactly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

BTW, I think it is helpful to add @iains's patches as parent versions to avoid 
conflicting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D122885#3426070 , @urnathan wrote:

> Ah, I'd had a thinko about what 1703 was.  that's superseded by 
> p1757wg21.link/p1857.  That's what is needed.  (your patch 
> makes more sense in the 1703 context)

Ha, okay. Yes, that makes more sense. Both papers are linked in 
https://clang.llvm.org/cxx_status.html. Sorry for the confusion, I'll have a 
look at 1857R3 instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

Ah, I'd had a thinko about what 1703 was.  that's superseded by p1757.  That's 
what is needed.  (your patch makes more sense in the 1703 context)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 ;')  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@-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 class module {...};

module x;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

I can;t really tell what you're doing.  C++20 lexes import () 
declarations as preprocessor directives.  the parser must also recognize only 
such declarations that came through from lexing such directives.  It is 
unrelated to #import.  GCC's implementation drops the preprocessor into 
directive mode if it sees 'module' or 'import' (optionally preceeded by 
'export') at the start of a line[1].  Those keywords are converted to 
unspellable internal tokens that the parser recognizes to parse module and 
import declarations.  An end-of-pragma token is injected at the end-of-line, 
and again the parser expects that after the terminating ';'. (it's leveraging 
the pragma machinery)

[1] it also peeks the next non-white-space token for '<', '", or 
, as specified in the std.

occurrences of 'module' and 'import' that are not lexed that way go through as 
regular identifier tokens, not keywords.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122885

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, iains, urnathan, ChuanqiXu.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As in: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1703r1.html

Posting this mostly to get some feedback on the general path taken.

>From reading the paper, I understand that the preprocessor should gain a 
>"import-keyword" token, but that is already taken by the GNU `#import` 
>extension.

The implementation here starts converting newlines to `eod` tokens when seeing 
a `export` or `import` token. However, that means ignoring the `eod` tokens 
when parsing an export block, an exported declaration or a non-header-unit 
module import.

Let me know what you think.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122885

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CXX/cpp/cpp.module/p1.cpp
  clang/test/CXX/lex/lex.pptoken/p3-2a.cpp
  clang/test/Modules/P1703R1.cpp

Index: clang/test/Modules/P1703R1.cpp
===
--- /dev/null
+++ clang/test/Modules/P1703R1.cpp
@@ -0,0 +1,103 @@
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: mkdir -p %t/system
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit \
+// RUN: -xc++-header header.h -o header.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -isystem system \
+// RUN: -xc++-system-header system-header.h -o system/system-header.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface dummy.cpp -o dummy.pcm
+
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=header.pcm \
+// RUN: -fmodule-file=system/system-header.pcm \
+// RUN: -fmodule-file=dummy.pcm \
+// RUN: -emit-module-interface -isystem system \
+// RUN: main.cpp -verify
+
+//--- header.h
+int foo(int);
+
+//--- system/system-header.h
+int foo2(int);
+
+//--- dummy.cpp
+export module dummy;
+
+//--- main.cpp
+export module main;
+#define IMPORT import
+#define EXPORT export
+#define HEADER "header.h"
+#define SYSTEM_HEADER 
+#define SEMI ;
+
+
+/// Fine.
+import "header.h";
+export import "header.h";
+
+
+import
+"header.h";// expected-error {{expected a module name after 'import'}}
+export import
+"header.h"; // expected-error {{expected a module name after 'import'}}
+export
+import "header.h"; // expected-error {{expected a module name after 'import'}}
+
+import "header.h"; import "header.h"; // expected-error {{import keyword must be at start of line}}
+  // expected-error@-1 {{extra tokens after module import}}
+
+import "header.h" SEMI // expected-error {{semicolon terminating header import declaration cannot be produced by a macro}}
+IMPORT "header.h"; // expected-error {{import keyword cannot be produced by a macro}}
+EXPORT import "header.h"; // expected-error {{export keyword cannot be produced by a macro}}
+import HEADER; /// Fine.
+
+
+import ;
+export import ;
+
+import
+; // expected-error {{expected a module name after 'import'}}
+IMPORT ;// expected-error {{import keyword cannot be produced by a macro}}
+EXPORT import ;// expected-error {{export keyword cannot be produced by a macro}}
+import SYSTEM_HEADER;
+
+
+
+/// Normal module imports are unaffected
+import dummy;
+import
+dummy;
+export
+import
+dummy;
+export import
+dummy;
+export
+import dummy;
+import dummy; import dummy;
+
+
+
+// TODO: The diagnostics here could be much better.
+export
+import "header.h"; // expected-error {{expected a module name after 'import'}}
+
+export import
+"header.h" // expected-error {{expected a module name after 'import'}}
+
+
+export void foo() {}
+export void
+baz(){}
+export
+void bar(){}
+
+export {
+  void a(){}
+}
Index: clang/test/CXX/lex/lex.pptoken/p3-2a.cpp
===
--- clang/test/CXX/lex/lex.pptoken/p3-2a.cpp
+++ clang/test/CXX/lex/lex.pptoken/p3-2a.cpp
@@ -56,26 +56,3 @@
 #define HEADER 
 // CHECK: import ;
 import HEADER;
-
-// CHECK: import ;
-import <
-foo
-  bar
->;
-
-// CHECK: import{{$}}
-// CHECK: {{^}};
-import
-<
-foo
-  bar
->;
-
-// CHECK: import{{$}}
-// CHECK: {{^}};
-import
-;
-
-#define IMPORT import 
-// CHECK: import ;
-IMPORT;
Index: clang/test/CXX/cpp/cpp.module/p1.cpp
===
--- clang/test/CXX/cpp/cpp.module/p1.cpp
+++ clang/test/CXX/cpp/cpp.module/p1.cpp
@@ -5,11 +5,11 @@
 // expected-error@+1 {{semicolon terminating header import declaration cannot be produced by a macro}}
 import "empty.h" SEMI // CHECK: import attrs.{{.*}};
 
-#define IMPORT import "empty.h"
-IMPORT; // CHECK: import attrs.{{.*}};
+#define IMPORT "empty.h"
+import IMPORT; // CHECK: import attrs.{{.*}};