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 scoped enum, should these enumerators get an `RIR_`
> > prefix added to them?
> I used a class enum with a stream operator instead. Not a big fan of prefixed
> enum ;-)
Totally fine by me, thank you!
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:383
+ "identifier %0 is reserved because %select{"
+ "|"
+ "it starts with '_' at global scope|"
----------------
You may want to add a comment here to alert people to the fact that passing 0
for %1 is not valid but the `|` exists to make the enum order match the
diagnostic. Otherwise it looks like it's possible to get a diagnostic
`identifier 'whatever' is reserved because `.
================
Comment at: clang/lib/AST/Decl.cpp:1105-1111
+ if (Name[1] == '_') {
+ return ReservedIdentifierStatus::StartsWithDoubleUnderscore;
+ }
+ if ('A' <= Name[1] && Name[1] <= 'Z') {
+ return ReservedIdentifierStatus::
+ StartsWithUnderscoreFollowedByCapitalLetter;
+ }
----------------
================
Comment at: clang/lib/AST/Decl.cpp:1118-1120
+ if (isa<TranslationUnitDecl>(DC)) {
+ return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+ }
----------------
================
Comment at: clang/lib/AST/Decl.cpp:1125-1127
+ if (LangOpts.CPlusPlus && Name.contains("__")) {
+ return ReservedIdentifierStatus::ContainsDoubleUnderscore;
+ }
----------------
================
Comment at: clang/lib/Sema/SemaDecl.cpp:5554
+ // Avoid warning twice on the same identifier, and don't warn on
redeclaration
+ // of system decl
+ if (D->getPreviousDecl())
----------------
aaron.ballman wrote:
>
Still missing a full stop at the end of the comment here.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:5557-5558
+ return;
+ if (!Context.getSourceManager().isInSystemHeader(D->getLocation()) &&
+ D->isReserved(getLangOpts()))
+ Diag(D->getLocation(), diag::warn_reserved_identifier) << D;
----------------
rsmith wrote:
> Swap the order of these checks; the "is reserved" check is faster and will
> usually allow us to short-circuit, whereas we're probably usually not in a
> system header and that check involves nontrivial work recursively decomposing
> the given source location.
Richard's comment is still unaddressed as well.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:13640
+ warnOnReservedIdentifier(New);
+
----------------
rsmith wrote:
> serge-sans-paille wrote:
> > serge-sans-paille wrote:
> > > rsmith wrote:
> > > > Is there somewhere more central you can do this, rather than repeating
> > > > it once for each kind of declaration? (Eg, `PushOnScopeChains`)
> > > That would be sane. I'll check that.
> > I tried PushOnScopeChains, and this does not capture all the required
> > declarations. I failed to find another place :-/
> What cases is it missing? Can we add calls to `PushOnScopeChains` to those
> cases (perhaps with a new flag to say "don't actually push it into scope"?)
> I'm not super happy about adding a new thing that all places that create
> declarations and add them to scope need to remember to do, to drive this
> warning. Cases are going to get forgotten that way.
I'm still wondering about this as well.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:545
+ if (!Context.getSourceManager().isInSystemHeader(IdentLoc)) {
+ ReservedIdentifierStatus Status = TheDecl->isReserved(getLangOpts());
+ if (Status != ReservedIdentifierStatus::NotReserved)
----------------
This check should be reversed as well.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93095/new/
https://reviews.llvm.org/D93095
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
