[PATCH] D93095: Introduce -Wreserved-identifier

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb83b23275b74: Introduce -Wreserved-identifier (authored by serge-sans-paille). Herald added a project: clang. Changed prior to commit:

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from the formatting nit and the question about the diagnostic, I think this LGTM! Comment at: clang/include/clang/Basic/DiagnosticGroups.td:637 def

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 341853. serge-sans-paille marked 10 inline comments as done. serge-sans-paille added a comment. Take into account latest reviews CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files:

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added a comment. In D93095#2663639 , @serge-sans-paille wrote: > Warn on friend functions. I failed to support friend classes, but they are > only declared and not defined, so that should be fine, right? They can still conflict, so I think we

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:383 + "identifier %0 is reserved because %select{" + "|" // passing 0 for %1 is not valid but we need that | to make the enum order match the diagnostic + "it starts with '_' at

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 334662. serge-sans-paille added a comment. Warn on friend functions. I failed to support friend classes, but they are only declared and not defined, so that should be fine, right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 334660. serge-sans-paille added a comment. Do not use lexical parent, as suggested by @rsmith Add test case for extern function worward-declared in function body, as suggested by @rsmith CHANGES SINCE LAST ACTION

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added inline comments. Comment at: clang/lib/AST/Decl.cpp:1097 + // ignored values) that we don't warn on it. + if (Name.size() <= 1) +return ReservedIdentifierStatus::NotReserved; serge-sans-paille wrote: > rsmith wrote: > > Would it make sense to

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille added inline comments. Comment at: clang/lib/AST/Decl.cpp:1097 + // ignored values) that we don't warn on it. + if (Name.size() <= 1) +return ReservedIdentifierStatus::NotReserved; rsmith wrote: > Would it make sense to move the rest of

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 334445. serge-sans-paille added a comment. Herald added a subscriber: dexonsmith. Another round of review :-) I addressed both the scattering of `warnOnReservedIdentifier` and the code split between `Decl.cpp` and `IdentifierInfo.cpp`. CHANGES

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:383 + "identifier %0 is reserved because %select{" + "|" // passing 0 for %1 is not valid but we need that | to make the enum order match the diagnostic + "it starts with '_' at global

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13640 + warnOnReservedIdentifier(New); + aaron.ballman wrote: > rsmith wrote: > > serge-sans-paille wrote: > > > serge-sans-paille wrote: > > > > rsmith wrote: > > > > > Is there

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 334261. serge-sans-paille marked 10 inline comments as done. serge-sans-paille added a comment. Address all the needed changes pointed out by @aaron.ballman *except* the most critical one on the call to warnOnReservedIdentifier being spread at

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Decl.h:82 +enum ReservedIdentifierReason { + StartsWithUnderscoreAtGlobalScope, + StartsWithDoubleUnderscore, serge-sans-paille wrote: > aaron.ballman wrote: > > Because this is not a

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 333812. serge-sans-paille added a comment. Fix typo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille marked 10 inline comments as done. serge-sans-paille added inline comments. Comment at: clang/include/clang/AST/Decl.h:82 +enum ReservedIdentifierReason { + StartsWithUnderscoreAtGlobalScope, + StartsWithDoubleUnderscore, aaron.ballman wrote:

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 333810. serge-sans-paille added a comment. Adress final(?) comments from @rsmith and @aaron.ballman : Don't warn on argument (or template argument) of top-level decl. Extra test cases Return an enumeration as part of the test CHANGES SINCE LAST

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Decl.h:82 +enum ReservedIdentifierReason { + StartsWithUnderscoreAtGlobalScope, + StartsWithDoubleUnderscore, Because this is not a scoped enum, should these enumerators get an `RIR_`

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 329361. serge-sans-paille added a comment. Fix some formatting Support literal operator CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 329318. serge-sans-paille added a comment. Updated error message to report the reason why an identifier is reserved. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files:

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 329242. serge-sans-paille added a comment. Patch rebased on main. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/test/Sema/reserved-identifier.c:49-50 +// FIXME: According to clang declaration context layering, _preserved belongs to +// the translation unit, so we emit a warning. It's unclear that's what the +// standard mandate, but

[PATCH] D93095: Introduce -Wreserved-identifier

mizvekov added a comment. There is a GNU extension where the linker provides symbols for section / start end, and using these with this patch would produce a warning. Example: ` [[gnu::section(foo)]] void bar() {} extern "C" void __start_foo(); extern "C" void __stop_foo(); CHANGES

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added inline comments. Comment at: clang/lib/AST/Decl.cpp:1087 + StringRef Name = II->getName(); + // '_' is a reserved identifier, but it's use is so common (e.g. to store + // ignored values) that we don't warn on it. Comment at:

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:377 + "%0 is a reserved identifier">, + InGroup, DefaultIgnore; + Quuxplusone wrote: > If you leave it like this, you //will// receive multiple bug reports per

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille marked an inline comment as done. serge-sans-paille added a comment. @rsmith I did my bet to address your comments. What do you think of current state? Comment at: clang/lib/Sema/SemaDecl.cpp:13640 + warnOnReservedIdentifier(New); +

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 320212. serge-sans-paille added a comment. Extra test cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille marked an inline comment as done. serge-sans-paille added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2421-2428 +// Perform a lookup at TUScope. If it succeeds, we're at global scope and a +// single '_' is enough to be reserved. +

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2421-2428 +// Perform a lookup at TUScope. If it succeeds, we're at global scope and a +// single '_' is enough to be reserved. +LookupResult IdentifierLookup(*this, II, SourceLocation(), +

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 319929. serge-sans-paille added a comment. Back to the previous version, as suggested by @rsmith . I made a few updates to `NamedDecl::isReserved` which get me close to the expected result, without too much overhead. CHANGES SINCE LAST ACTION

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2421-2428 +// Perform a lookup at TUScope. If it succeeds, we're at global scope and a +// single '_' is enough to be reserved. +LookupResult IdentifierLookup(*this, II, SourceLocation(), +

[PATCH] D93095: Introduce -Wreserved-identifier

rsmith added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2421-2428 +// Perform a lookup at TUScope. If it succeeds, we're at global scope and a +// single '_' is enough to be reserved. +LookupResult IdentifierLookup(*this, II, SourceLocation(), +

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 317677. serge-sans-paille added a comment. As suggested by @aaron.ballman base the detection of top-level-ness on `Sema::LookupName` to avoid re-implementing the wheel. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille added inline comments. Comment at: clang/lib/AST/Decl.cpp:1099 +const DeclContext *CurrentContext = getDeclContext(); +while (true) { + if (isa(CurrentContext)) aaron.ballman wrote: > Rather than trying to manually decide whether

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/lib/AST/Decl.cpp:1099 +const DeclContext *CurrentContext = getDeclContext(); +while (true) { + if (isa(CurrentContext)) Rather than trying to manually decide whether the name will be exposed at

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 315390. serge-sans-paille marked 2 inline comments as done. serge-sans-paille added a comment. Ignore forward declaration of tagdecl CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files:

[PATCH] D93095: Introduce -Wreserved-identifier

Quuxplusone added inline comments. Comment at: clang/test/Sema/reserved-identifier.cpp:77 + +long double operator"" _BarbeBleue(long double) // no-warning +{ This should get a warning, since it's using an identifier "reserved for any use."

[PATCH] D93095: Introduce -Wreserved-identifier

hubert.reinterpretcast added inline comments. Comment at: clang/test/Sema/reserved-identifier.cpp:40 + return foo__bar(); // no-warning +} serge-sans-paille wrote: > aaron.ballman wrote: > > aaron.ballman wrote: > > > You should also have some tests for: > > >

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille marked 2 inline comments as done. serge-sans-paille added inline comments. Comment at: clang/test/Sema/reserved-identifier.cpp:58 +// we skip this one because it's not at top-level. +int _barbatruc; // no-warning +} aaron.ballman wrote: > This

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 315373. serge-sans-paille added a comment. Update codebase and testbed to reflect recent discussion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille marked an inline comment as done. serge-sans-paille added inline comments. Comment at: clang/test/Sema/reserved-identifier.c:9 +int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}} + +// This one is explicitly skipped by

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 315313. serge-sans-paille added a comment. Address some of the review CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h

[PATCH] D93095: Introduce -Wreserved-identifier

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:377 + "%0 is a reserved identifier">, + InGroup, DefaultIgnore; + If you leave it like this, you //will// receive multiple bug reports per year about how in some

[PATCH] D93095: Introduce -Wreserved-identifier

aaron.ballman added inline comments. Comment at: clang/test/Sema/reserved-identifier.c:9 +int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}} + +// This one is explicitly skipped by -Wreserved-identifier Can you add a test

[PATCH] D93095: Introduce -Wreserved-identifier

serge-sans-paille updated this revision to Diff 314329. serge-sans-paille added a comment. Rebased on main. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93095/new/ https://reviews.llvm.org/D93095 Files: clang/include/clang/AST/Decl.h clang/include/clang/Basic/DiagnosticGroups.td