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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
