[PATCH] D93095: Introduce -Wreserved-identifier

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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