rsmith added a comment.
In D93095#2663639 <https://reviews.llvm.org/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 should warn. For example:
// user code
struct A { friend struct _b; };
// system header
enum _b {}; // error
================
Comment at: clang/include/clang/AST/Decl.h:81-82
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+ ReservedIdentifierStatus Status) {
+ return DB << static_cast<int>(Status);
----------------
This should be declared with the definition of `ReservedIdentifierStatus` -- or
simply removed. We usually include the cast at the diagnostic emission site,
which I find makes it locally clear that we're making an assumption about the
numerical values of the enumerators.
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:798
+
+def ReservedIdAsSymbol : DiagGroup<"reserved-extern-identifier">;
+def ReservedIdentifier : DiagGroup<"reserved-identifier",
----------------
Some of the uses of this warning are for non-external identifiers. It'd also
be nice for the diagnostics in this area to have consistent names. Currently we
have: `-Wreserved-id-macro`, `-Wreserved-extern-identifier`,
`-Wreserved-identifier`. I'd like to see either consistent use of "identifier"
or consistent use of "id' (preferably "identifier") and consistent word order.
================
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;
----------------
aaron.ballman wrote:
> 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)?
Should we instead be asking whether `CalleeDecl` has a reserved name, now that
we can?
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:746
+ ReservedIdentifierStatus Status = Id->isReserved(SemaRef.getLangOpts());
// Ignore reserved names for compiler provided decls.
----------------
Should we be using `ND->isReserved` here?
================
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;
----------------
aaron.ballman wrote:
>
I'm surprised we don't warn in user headers. Is that intentional?
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
