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 global scope|"
----------------
rsmith wrote:
> This is hopefully more useful to future readers and also terser. (I like
> including <ERROR> in the diagnostic in the bad case because if it ever
> happens we're more likely to get a bug report that way.)
Ooh, I like that suggestion, thanks!
================
Comment at: clang/lib/AST/Decl.cpp:1096
+
+ // Walk up the lexical parents to determine if we're at TU level or not.
+ if (isa<ParmVarDecl>(this) || isTemplateParameter())
----------------
This comment should be updated -- we no longer walk the lexical parents.
================
Comment at: clang/lib/AST/Decl.cpp:1084
+ const IdentifierInfo *II = nullptr;
+ if (const auto *FD = dyn_cast<FunctionDecl>(this))
+ II = FD->getLiteralIdentifier();
----------------
rsmith wrote:
> Please reorder the literal operator case after the plain `getIdentifier`
> case. I think this'd be more readable if the common and expected case is
> first, and it doesn't make any functional difference since a literal operator
> doesn't have a "regular" identifier name.
It looks like this code still needs to move.
================
Comment at: clang/lib/Basic/IdentifierTable.cpp:297
+
+ return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+ }
----------------
I think comments here may help -- we don't know that the identifier is at
global scope within this call, so the return value looks a bit strange. This is
a case where the enumerator name is a bit awkward in this context but makes a
lot of sense in the context of the check.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4051-4052
if (const auto *Id = CalleeDecl->getIdentifier())
- if (Id->isReservedName())
+ if (Id->isReserved(CGM.getLangOpts()) !=
+ ReservedIdentifierStatus::NotReserved)
return;
----------------
There's a slight functionality change here in that the code used to allow
identifiers that start with a single underscore followed by a lowercase letter
and that now early returns. Is that expected (and tested)?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:5564-5565
+ ReservedIdentifierStatus Status = D->isReserved(getLangOpts());
+ if (Status != ReservedIdentifierStatus::NotReserved)
+ if (Context.getSourceManager().isInMainFile(D->getLocation()))
+ Diag(D->getLocation(), diag::warn_reserved_extern_symbol) << D << Status;
----------------
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10998
PushDeclContext(NamespcScope, Namespc);
+
return Namespc;
----------------
Spurious whitespace change.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12673
ActOnDocumentableDecl(NewND);
+
return NewND;
----------------
Spurious whitespace change.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16743
+ warnOnReservedIdentifier(ND);
+
----------------
Doesn't `PushOnScopeChains()` do this already?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:545
+ if (!Context.getSourceManager().isInSystemHeader(IdentLoc)) {
+ ReservedIdentifierStatus Status = TheDecl->isReserved(getLangOpts());
+ if (Status != ReservedIdentifierStatus::NotReserved)
----------------
aaron.ballman wrote:
> This check should be reversed as well.
This still needs to be reversed so we check for system headers after checking
the reserved status.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1680-1681
+ for (NamedDecl *P : Params)
+ warnOnReservedIdentifier(P);
+
----------------
rsmith wrote:
> Again, it'd be more consistent to do this when we finish creating the
> declaration and push it into scope, for all kinds of declaration.
Is this handled by `PushOnScopeChains()`?
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
