[PATCH] D136953: [C++20] Diagnose invalid and reserved module names

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

In D136953#3892077 <https://reviews.llvm.org/D136953#3892077>, @cor3ntin wrote:

> In D136953#3892060 <https://reviews.llvm.org/D136953#3892060>, @erichkeane wrote:
>
>> I'll leave it to the modules experts to decide whether they're happy with this, but I had a drive-by.
>>
>> Also, I see none of the tests validate 'import', just 'export'.  Based on the standard, BOTH are illegal, right :D

Heh, Erich is joking about my malicious reading of the standard where module-name has blanket wording for what is reserved, but that same grammar production is used with importing modules. e.g., a malicious reading of the standard could make you think `import module std;` is also invalid because it's using a reserved module name.

> `export module export` is perfectly ~~fine~~ conforming

Nope, that's not going to compile because `export` is a real keyword instead of an identifier masquerading as a keyword like `module` and `import`.



================
Comment at: clang/lib/Sema/SemaModule.cpp:248
+  for (auto Part : Path) {
+    int Reason = -1;
+    const IdentifierInfo *II = Part.first;
----------------
erichkeane wrote:
> Urgh, could you make this an enum?
Absolutely.


================
Comment at: clang/lib/Sema/SemaModule.cpp:257
+    else if (PartName.startswith("std") &&
+             (PartName.size() == 3 || isDigit(PartName.drop_front(3)[0])))
+      Reason = /*reserved*/ 1;
----------------
cor3ntin wrote:
> 
lol but my way is so much more complicated, so it must be more right... ;-)


================
Comment at: clang/lib/Sema/SemaModule.cpp:262
+    // diagnose (because we expect system headers to use reserved identifiers).
+    if (Reason != -1 && !getSourceManager().isInSystemHeader(Part.second)) {
+      Diag(Part.second, diag::err_invalid_module_name) << Part.first << Reason;
----------------
cor3ntin wrote:
> I think `module` in a standard header is still invalid. We could not do the check in a  system header at all
Good catch, that's a think-o on my part!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136953