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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb8ceb9f4e4bd: [C++20] Diagnose invalid and reserved module names (authored by aaron.ballman). Changed prior to commit: https://reviews.llvm.org/D1

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

aaron.ballman added a comment. I believe we now have consensus on the patch. I'll land it in a few days in case any reviewers want to weigh in now that the discussion has stabilized. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first-

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: > p

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || philnik wrote: > a

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: > p

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || philnik wrote: > a

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: > p

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || philnik wrote: > a

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: > p

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || philnik wrote: > a

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: > p

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

aaron.ballman marked 2 inline comments as done. aaron.ballman added inline comments. Comment at: clang/test/Modules/reserved-names-1.cpp:33 +expected-error {{module declaration must occur at the start of the translation unit}} + +// Show that

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

aaron.ballman updated this revision to Diff 472052. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updated based on review feedback. Added an additional test case, reworked the comments in one of the tests as well. CHANGES SINCE LAST ACTION https://reviews.llv

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

aaron.ballman marked 9 inline comments as done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName

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

philnik added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || ChuanqiXu wrote: > aaron

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

ChuanqiXu accepted this revision. ChuanqiXu added a comment. LGTM if we add a test for `foo.std`. Comment at: clang/docs/ReleaseNotes.rst:351 + export declaration. Both are diagnosed as an error, but the diagnostic is + suppressed for use of reserved names in a system header.

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

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName

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

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || erichkeane wrote: > Ch

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

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName

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

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || ChuanqiXu wrote: > aa

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

ChuanqiXu added inline comments. Comment at: clang/test/Modules/reserved-names-1.cpp:25 + expected-error {{module declaration must occur at the start of the translation unit}} +export module std.foo;// expected-error {{'std' is a reserved name for a modu

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

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang/test/Modules/reserved-names-1.cpp:25 + expected-error {{module declaration must occur at the start of the translation unit}} +export module std.foo;// e

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

ChuanqiXu added inline comments. Comment at: clang/test/Modules/reserved-names-1.cpp:25 + expected-error {{module declaration must occur at the start of the translation unit}} +export module std.foo;// expected-error {{'std' is a reserved name for a modu

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

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || aaron.ballman wrote: >

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

aaron.ballman updated this revision to Diff 471988. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updated based on review feedback: - Fixed a typo - Clarified the release note - Added a test case for `std.foo` being reserved CHANGES SINCE LAST ACTION https://r

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

aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. In D136953#3893039 , @tschuett wrote: > Are malformed imports an issue or are they already covered? There is limit > test coverage for import. Am I missing something? > > im

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || ChuanqiXu wrote: >

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

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || cor3ntin wrote: > Chua

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

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:347 Fixes `Issue 57562 `_. +- Clang now diagnoses use of invalid or reserved module names. Both are + diagnosed as an error, but the diagnostic is supp

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

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:282 + StringRef FirstComponentName = Path[0].first->getName(); + if (!getSourceManager().isInSystemHeader(Path[0].second) && + (FirstComponentName == "std" || ChuanqiXu wrote: > std

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

ChuanqiXu added a comment. In D136953#3892578 , @aaron.ballman wrote: > I think the standards wording here is a bit unclear as to what's intended and > I had misunderstood the wording around which part of the path needs to be > checked. Specifically: >

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

tschuett added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:148 +/// Tests whether the given identifier is reserved as a module name and +/// diagnoses if it is. Returs true if a diagnostic is emitted and false +/// otherwise. Returns CHANGES SINC

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

tschuett added a comment. Are malformed imports an issue or are they already covered? There is limit test coverage for import. Am I missing something? import import; import module; CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136953/new/ https://reviews.llvm.org/D136953

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

cor3ntin accepted this revision. cor3ntin added a comment. This revision is now accepted and ready to land. Modulo the import typo. I'm happy with this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136953/new/ https://reviews.llvm.org/D136953 __

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

tschuett added a comment. Thx. Comment at: clang/test/Modules/reserved-names-1.cpp:42 + +// We can still use a reserved name on imoport. +import std; // expected-error {{module 'std' not found}} import? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D13

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:241-248 + // C++2b [module.unit]p1: ... The identifiers module and import shall not + // appear as identifiers in a module-name or module-partition. All + // module-names either beginning with an id

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

aaron.ballman updated this revision to Diff 471628. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updating based on review feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136953/new/ https://reviews.llvm.org/D136953 Files: clang/docs/ReleaseNo

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

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:241-248 + // C++2b [module.unit]p1: ... The identifiers module and import shall not + // appear as identifiers in a module-name or module-partition. All + // module-names either beginning with an identif

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

aaron.ballman updated this revision to Diff 471608. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updated based on review feedback and reflection. I think the standards wording here is a bit unclear as to what's intended and I had misunderstood the wording around

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

erichkeane added inline comments. 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: > erichkeane wr

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

cor3ntin added inline comments. 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; erichkeane wrote: > aaron.ballman

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

erichkeane added inline comments. 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; aaron.ballman wrote: > cor3ntin

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

aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. In D136953#3892077 , @cor3ntin wrote: > In D136953#3892060 , @erichkeane > wrote: > >> I'll leave it to the modules experts to decide

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

cor3ntin added a comment. In 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

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

erichkeane added a comment. 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 Comment at: clang/lib/Sema/Sem

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

cor3ntin added inline comments. 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; Comment at: c