[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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:
  https://reviews.llvm.org/D93095?vs=341853&id=342681#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93095/new/

https://reviews.llvm.org/D93095

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Preprocessor/macro-reserved.c
  clang/test/Preprocessor/macro-reserved.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+}

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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 KeywordAsMacro : DiagGroup<"keyword-macro">;
-def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
 

Do we want to keep this old name around as an alias to the new name (this makes 
upgrades easier on users who mention the diagnostic by name in their build 
script or within diagnostic pragmas)?



Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = getIdentifier();
+  if(!II)
+if (const auto *FD = dyn_cast(this))

The formatting issue is worth fixing.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Preprocessor/macro-reserved.c
  clang/test/Preprocessor/macro-reserved.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _bar

[PATCH] D93095: Introduce -Wreserved-identifier

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

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 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(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

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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  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(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(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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D93095

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long dou

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D93095/new/

https://reviews.llvm.org/D93095

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+

[PATCH] D93095: Introduce -Wreserved-identifier

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

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 move the rest of this function to a member on 
> > `IdentifierInfo`? That is, the rest of this function would become:
> > 
> > ```
> > ReservedIdentifierStatus Status = II->isReserved(LangOpts);
> > if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope &&
> > isa(this) || isTemplateParameter() ||
> > !getDeclContext()->getRedeclContext()->isTranslationUnit())
> >   return ReservedIdentifierStatus::NotReserved;
> > return Status;
> > ```
> > 
> > That way, we should be able to reuse the same implementation to detect 
> > reserved macros too.
> I did the `II->isReserved(LangOpts)` part, but the updated condition 
> `!getDeclContext()->getRedeclContext()->isTranslationUnit()` fails the 
> validation. I did already spend a lot of time getting one that catches the 
> behavior we wanted, so 'm not super-eager to investigate why this change 
> would not work ;-)
The current lexical context check does not look correct -- the lexical context 
is not relevant here, because the language rule is constrained by the semantic 
context. What test cases does the new condition fail?

Please also add something like the testcases from my other comment; I can't 
find any tests for friends or for local extern declarations in the current set 
of tests, and that's the kind of corner case that would be affected by this.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 this function to a member on 
> `IdentifierInfo`? That is, the rest of this function would become:
> 
> ```
> ReservedIdentifierStatus Status = II->isReserved(LangOpts);
> if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope &&
> isa(this) || isTemplateParameter() ||
> !getDeclContext()->getRedeclContext()->isTranslationUnit())
>   return ReservedIdentifierStatus::NotReserved;
> return Status;
> ```
> 
> That way, we should be able to reuse the same implementation to detect 
> reserved macros too.
I did the `II->isReserved(LangOpts)` part, but the updated condition 
`!getDeclContext()->getRedeclContext()->isTranslationUnit()` fails the 
validation. I did already spend a lot of time getting one that catches the 
behavior we wanted, so 'm not super-eager to investigate why this change would 
not work ;-)


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/IdentifierTable.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Basic/IdentifierTable.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+

[PATCH] D93095: Introduce -Wreserved-identifier

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

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 scope|"

This is hopefully more useful to future readers and also terser. (I like 
including  in the diagnostic in the bad case because if it ever happens 
we're more likely to get a bug report that way.)



Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = nullptr;
+  if (const auto *FD = dyn_cast(this))
+II = FD->getLiteralIdentifier();

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.



Comment at: clang/lib/AST/Decl.cpp:1097
+  // ignored values) that we don't warn on it.
+  if (Name.size() <= 1)
+return ReservedIdentifierStatus::NotReserved;

Would it make sense to move the rest of this function to a member on 
`IdentifierInfo`? That is, the rest of this function would become:

```
ReservedIdentifierStatus Status = II->isReserved(LangOpts);
if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope &&
isa(this) || isTemplateParameter() ||
!getDeclContext()->getRedeclContext()->isTranslationUnit())
  return ReservedIdentifierStatus::NotReserved;
return Status;
```

That way, we should be able to reuse the same implementation to detect reserved 
macros too.



Comment at: clang/lib/AST/Decl.cpp:1113-1119
+if (!isa(this) && !isTemplateParameter()) {
+  const DeclContext *DC = getLexicalDeclContext();
+  while (DC->isTransparentContext())
+DC = DC->getLexicalParent();
+  if (isa(DC))
+return ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope;
+}

This can be simplified a bit; I suggested edits above. Also, we want the 
semantic parent here, not the lexical parent, in order to warn on cases like

```
struct A {
  friend struct _b; // warning, starts with underscore at global scope
  friend void _f(); // warning, starts with underscore at global scope
};
void g() {
  void _h(); // warning, starts with underscore at global scope
  extern int _n; // warning, starts with underscore at global scope
}
```


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 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.
I'll investigate that one more deeply, I do share your concern.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 several 
place in Sema. I'll try to find a better approach for that particular point.


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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double oper

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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-
+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(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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // expected-warning {{identifier 'operator""_BarbeBleue' is reserved because it starts with '_' followed by a capital letter}}
+{
+  return 0.;
+}
+
+struct _BarbeRouge { // expected-warning {{identifier '_BarbeRouge' is reserved because it starts

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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:
> 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 ;-)



Comment at: clang/test/Sema/reserved-identifier.c:53
+
+extern char *_strdup(const char *); // expected-warning {{'_strdup' is a 
reserved identifier}}
+

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > This is a case I don't think we should warn on. As some examples showing 
> > > this sort of thing in the wild:
> > > 
> > > https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/lz.cpp
> > > https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/svc1.cpp
> > You mean, specifically for `_strdup`? In general, this looks exactly the 
> > like the kind of declaration we'd want to warn on. If we don't want a 
> > warning here, we could perhaps recognize `_strdup` as a builtin lib 
> > function. (I think they shouldn't warn, because we'll inject a builtin 
> > declaration, so this would be a redeclaration. We should test that!)
> > You mean, specifically for _strdup?
> 
> Yes, but it's a more general pattern than just `_strdup`. C code (esp older C 
> code) will sometimes add an external declaration to avoid having to use a 
> `#include` for just one symbol. Additionally, some popular platforms (like 
> Windows) add a bunch of functions in the implementation namespace like 
> `_strdup` (and many, many others).
> 
> Perhaps an option would be to always warn, and if we find that users run into 
> this situation a lot in practice, we could split the diagnostic so that users 
> can control whether to warn on forward declarations of functions.
yeah,that would require checking if the name with trailing underscores is a 
libc function. I agree we should wait for more user feedback to install such 
check. 


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' followed by a capital letter}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' followed by a capital letter}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '__'}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' followed by a capital letter}}
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+struct BarbeNoire {};
+
+template  // no-warning
+struct BarbeJaune {};
+
+template  // expected-warning {{'__' is reserved because it starts with '__'}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' followed by a capital letter}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '__'}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '__'}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' followed by a capital letter}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' followed by a capital letter}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' followed by a capital letter}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // expected-warning {{identifier 'operator""_BarbeBleue' is reserved beca

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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_` 
prefix added to them?



Comment at: clang/include/clang/AST/Decl.h:368-369
+  /// given language.
+  bool isReserved(const LangOptions &,
+  ReservedIdentifierReason *Reason = nullptr) const;
+

Any reason not to return the enumeration as the result? We could add 
`NotReserved = 0` to the enumeration so that calls to `D->isReserved()` will do 
the right thing.

(Also, might as well name the `LangOptions` parameter here.)



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:790
+
+def ReservedIdAsSymbol : DiagGroup<"reserved-extern-identifier">;
+def ReservedIdentifier : DiagGroup<"reserved-identifier",

Were you planning to use this new diagnostic group?



Comment at: clang/lib/AST/Decl.cpp:1084
+  const IdentifierInfo *II = nullptr;
+  if (auto *FD = dyn_cast(this))
+II = FD->getLiteralIdentifier();

Might as well address the lint warning.



Comment at: clang/lib/AST/Decl.cpp:1104-1105
+// Walk up the lexical parents to determine if we're at TU level or not.
+const DeclContext * DC = getLexicalDeclContext();
+while(DC->isTransparentContext())
+  DC = DC->getLexicalParent();




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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,84 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{identifier 'foo__bar' is reserved because it contains '__'}}
+static int _bar() { return 0; } // expected-warning {{identifier '_bar' is reserved because it starts with '_' at global scope}}
+static int _Bar() { return 0; } // expected-warning {{identifier '_Bar' is reserved because it starts with '_' at global scope}}
+int _barbouille() { return 0; } // expected-warning {{identifier '_barbouille' is reserved because it starts with '_' at global scope}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{identifier '_Reserved' is reserved because it starts with '_' at global scope}}
+  unsigned int __1 =   // expected-warning {{identifier '__1' is reserved because it starts with '__'}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{identifier '__toucan' is reserved because it starts with '_' at global scope}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{identifier '_Barbotine' is reserved because it starts with '_' at global scope}}
+
+template  // expected-warning {{'__' is reserved because it starts with '_' at global scope}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is reserved because it starts with '_' at global scope}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{identifier '_Barbidur' is reserved because it starts with '_' at global scope}}
+
+struct __barbidou {}; // expected-warning {{identifier '__barbidou' is reserved because it starts with '__'}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{identifier '__barbouille' is reserved because it starts with '__'}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{identifier '__babar' is reserved because it starts with '__'}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{identifier '__barbapapa' is reserved because it starts with '_' at global scope}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{identifier '_Barbalala' is reserved because it starts with '_' followed by a capital letter}}
+};
+
+enum class __menu { // expected-warning {{identifier '__menu' is reserved because it starts with '_' at global scope}}
+  __some,   // expected-warning {{identifier '__some' is reserved because it starts with '__'}}
+  _Other,   // expected-warning {{identifier '_Other' is reserved because it starts with '_' followed by a capital letter}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{identifier '_Menu' is reserved because it starts with '_' at global scope}}
+  _OtheR_,   // expected-warning {{identifier '_OtheR_' is reserved because it starts with '_' at global scope}}
+  _other_// expected-warning {{identifier '_other_' is reserved because it starts with '_' at global scope}}
+};
+
+enum {
+  __some, // expected-warning {{identifier '__some' is reserved because it starts with '_' at global scope}}
+  _Other, // expected-warning {{identifier '_Other' is reserved because it starts with '_' at global scope}}
+  _other  // expected-warning {{identifier '_other' is reserved because it starts with '_' at global scope}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{identifier '_Barbamama' is reserved because it starts with '_' at global scope}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // expected-warning {{identifier 'operator""_BarbeBleue' is reserved because it starts with '_' at global scope}}
+{
+  return 0.;
+}
+
+struct _BarbeRouge { // expected-warning {{identifier '_BarbeRouge' is reserved because it starts with '_' at global scope}}
+} p;
+struct

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
+
+struct _BarbeRouge {} p; // expected-warning {{'_BarbeRouge' is a reserved identifier}}
+struct _BarbeNoire {}* q; // expected-warning {{'_BarbeNoire' is a reserved identifier}}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __rese

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
+
+struct _BarbeRouge {} p; // expected-warning {{'_BarbeRouge' is a reserved identifier}}
+struct _BarbeNoire {}* q; // expected-warning {{'_BarbeNoire' is a reserved identifier}}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__reserve

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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 it's such a corner case we can live with it.
+void func(struct _preserved { int a; } r) {} // expected-warning 
{{'_preserved' is a reserved identifier}}

rsmith wrote:
> aaron.ballman wrote:
> > I think we're correct to warn on this. Because the type specifier appears 
> > within the parameter list, it has function prototype scope per C2x 6.2.1p4. 
> > However, the identifier `_preserved` is put into the struct name space, 
> > which hits the "in the tag name spaces" part of: "All identifiers that 
> > begin with an underscore are reserved for use as identifiers with file 
> > scope in both the ordinary and tag name spaces".
> `_preserved` is not an identifier with file scope, so I think we shouldn't 
> warn here. Perhaps the problem is that we're doing this check before the 
> struct gets reparented into the function declaration (which only happens 
> after we finish parsing all the parameters and build the function 
> declaration).
> 
> We could perhaps check the `Scope` in addition to checking the `DeclContext` 
> to detect such cases.
> _preserved is not an identifier with file scope, so I think we shouldn't warn 
> here. 

Hmm, my thinking was that if the implementation added a conflicting definition 
of what `_preserved` is, it'd break the user's code. But you're right, the 
identifier is in the tag name space but not with file scope, so not warning is 
correct.



Comment at: clang/test/Sema/reserved-identifier.c:53
+
+extern char *_strdup(const char *); // expected-warning {{'_strdup' is a 
reserved identifier}}
+

rsmith wrote:
> aaron.ballman wrote:
> > This is a case I don't think we should warn on. As some examples showing 
> > this sort of thing in the wild:
> > 
> > https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/lz.cpp
> > https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/svc1.cpp
> You mean, specifically for `_strdup`? In general, this looks exactly the like 
> the kind of declaration we'd want to warn on. If we don't want a warning 
> here, we could perhaps recognize `_strdup` as a builtin lib function. (I 
> think they shouldn't warn, because we'll inject a builtin declaration, so 
> this would be a redeclaration. We should test that!)
> You mean, specifically for _strdup?

Yes, but it's a more general pattern than just `_strdup`. C code (esp older C 
code) will sometimes add an external declaration to avoid having to use a 
`#include` for just one symbol. Additionally, some popular platforms (like 
Windows) add a bunch of functions in the implementation namespace like 
`_strdup` (and many, many others).

Perhaps an option would be to always warn, and if we find that users run into 
this situation a lot in practice, we could split the diagnostic so that users 
can control whether to warn on forward declarations of functions.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[Matheus Izvekov via Phabricator via cfe-commits]

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 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

[PATCH] D93095: Introduce -Wreserved-identifier

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

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: clang/lib/AST/Decl.cpp:1096
+// Walk up the lexical parents to determine if we're at TU level or not.
+const DeclContext * DC = getLexicalDeclContext();
+while(DC->isTransparentContext())

aaron.ballman wrote:
> You may want to run the patch through clang-format.
The lexical parent doesn't matter here; what we care about is whether this 
declaration would conflict with a declaration in the global namespace, which 
means we should check the semantic parent. So we want 
`getDeclContext()->getRedeclContext()->isTranslationUnit()`.

For a variable or function, you should also check `isExternC()`, because 
`extern "C"` functions and variables (declared in any scope) conflict with 
variables and functions with the same name declared in the global namespace 
scope.



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;

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.



Comment at: clang/lib/Sema/SemaDecl.cpp:17114
 
+warnOnReservedIdentifier(FD);
+

It would be more consistent with the other calls to this function to call this 
when we create each individual field, not when we finalize the record 
definition.



Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+

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.



Comment at: clang/lib/Sema/SemaTemplate.cpp:1680-1681
 
+  for (NamedDecl *P : Params)
+warnOnReservedIdentifier(P);
+

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.



Comment at: clang/test/Sema/reserved-identifier.c:13
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a 
reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved 
identifier}}

It'd be useful to test that we don't diagnose

```
void foo(unsigned int _not_reserved) { ... }
```



Comment at: clang/test/Sema/reserved-identifier.c:31
+struct _Zebulon; // expected-warning {{'_Zebulon' is a reserved identifier}}
+struct _Zebulon2 {}* p; // expected-warning {{'_Zebulon2' is a reserved 
identifier}}
+

You don't seem to have a test for `TUK_Reference`:

```
struct _Zebulon3 *p;
```

(Nor for `TUK_Friend`.)



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 it's such a corner case we can live with it.
+void func(struct _preserved { int a; } r) {} // expected-warning 
{{'_preserved' is a reserved identifier}}

aaron.ballman wrote:
> I think we're correct to warn on this. Because the type specifier appears 
> within the parameter list, it has function prototype scope per C2x 6.2.1p4. 
> However, the identifier `_preserved` is put into the struct name space, which 
> hits the "in the tag name spaces" part of: "All identifiers that begin with 
> an underscore are reserved for use as identifiers with file scope in both the 
> ordinary and tag name spaces".
`_preserved` is not an identifier with file scope, so I think we shouldn't warn 
here. Perhaps the problem is that we're doing this check before the struct gets 
reparented into the function declaration (which only happens after we finish 
parsing all the parameters and build the function declaration).

We c

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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 year 
> about how in some contexts the compiler diagnoses `_X` and `_x` equally, and 
> in other contexts the compiler diagnoses `_X` but fails to diagnose `_x`.
> 
> You should make two diagnostics: `-Wreserved-extern-identifier` and 
> `-Wreserved-pp-identifier` (or something along those lines), and their 
> messages should be carefully worded to explain //why// the warning is 
> happening — "identifier %0 is reserved for use at file scope," "identifier %0 
> is reserved in all contexts," etc.
> 
> Btw, the reason some identifiers (like `_X`) are reserved in all contexts is 
> so that the implementation can define them as macros (e.g. header guards).
I think splitting the diagnostic into two groups could be useful, but I'd still 
like the groups to be under a common grouping so I can enable/disable all 
reserved identifier warnings at once.

As for adding the extra rationale to the diagnostic message: I think if we want 
to add extra wording, it should be more about how the user can fix the name and 
less about why the standard reserves something. e.g., `"identifier %0 is 
reserved because %select{it starts with '__'|it contains '__'|it is the same 
identifier as a keyword|etc}0"`. Knowing that something is reserved at file 
scope is somewhat helpful, but knowing how to change the name to appease the 
compiler is too.



Comment at: clang/lib/AST/Decl.cpp:1096
+// Walk up the lexical parents to determine if we're at TU level or not.
+const DeclContext * DC = getLexicalDeclContext();
+while(DC->isTransparentContext())

You may want to run the patch through clang-format.



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())





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10018
   if (DeclaratorDecl *DD = dyn_cast(D)) {
-for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i)
+for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i) {
   ParameterLists.push_back(DD->getTemplateParameterList(i));

Unrelated changes?



Comment at: clang/lib/Sema/SemaStmt.cpp:544
 
+  if (!Context.getSourceManager().isInSystemHeader(IdentLoc) &&
+  TheDecl->isReserved(getLangOpts()))

Can you not call `warnOnReservedIdentifier(TheDecl)` in this case?



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 it's such a corner case we can live with it.
+void func(struct _preserved { int a; } r) {} // expected-warning 
{{'_preserved' is a reserved identifier}}

I think we're correct to warn on this. Because the type specifier appears 
within the parameter list, it has function prototype scope per C2x 6.2.1p4. 
However, the identifier `_preserved` is put into the struct name space, which 
hits the "in the tag name spaces" part of: "All identifiers that begin with an 
underscore are reserved for use as identifiers with file scope in both the 
ordinary and tag name spaces".



Comment at: clang/test/Sema/reserved-identifier.c:53
+
+extern char *_strdup(const char *); // expected-warning {{'_strdup' is a 
reserved identifier}}
+

This is a case I don't think we should warn on. As some examples showing this 
sort of thing in the wild:

https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/lz.cpp
https://codesearch.isocpp.org/actcd19/main/m/mcrl2/mcrl2_201409.0-1/3rd-party/svc/source/svc1.cpp



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

serge-sans-paille wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > Can you add a test that we do not warn on an extern declaration? e.g.,
> > > ```
> > > extern char *_strdup(const char *);
> > > ```
> > > This is sometimes used (esp in older C code bases) to avoid having to 
> > > include an entire header to scrape one declaration out of it, and there 
> > > are popular libraries (like the MSVC CRT) that have APIs starting with a 
> > > single underscore and lowercase letter.
> > > 
> > > The idea here is: if it's an extern declaration, the identifier is 
> > > "owned" by some other declaration and this is not likely something the 

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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);
+

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 :-/



Comment at: clang/lib/Sema/SemaDecl.cpp:16296
   } else {
+if (TUK == TUK_Definition)
+  warnOnReservedIdentifier(New);

serge-sans-paille wrote:
> rsmith wrote:
> > Why do we not diagnose the other possible `TagUseKind`s? `struct _foo;` and 
> > `struct _foo *p;` both use reserved identifiers too.
> We have a test case for `struct _foo` and its correctly diagnosed. I'll 
> double check for pointer / reference too.
Test case added for pointers, works like a charm.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
+
+struct _BarbeRouge {} p; // expected-warning {{'_BarbeRouge' is a reserved identifier}}
+struct _BarbeNoire {}* q; // expected-warning {{'_BarbeNoire' is a reserved identifier}}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__reserved' is 

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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.
+LookupResult IdentifierLookup(*this, II, SourceLocation(),
+  Sema::LookupAnyName,
+  Sema::ForExternalRedeclaration);
+IdentifierLookup.suppressDiagnostics();
+if (LookupName(IdentifierLookup, TUScope))

rsmith wrote:
> I don't understand why name lookup is involved here. Whether an identifier is 
> reserved doesn't depend on whether it's already been declared in the global 
> namespace or not. It's valid to declare `_foo` in some user-defined namespace 
> regardless of whether there's already a `_foo` in the global namespace, and 
> it's invalid to declare `_foo` in the global namespace regardless of whether 
> there's already a `_foo` in the global namespace.
> 
> If you're trying to detect whether the name is being introduced in the global 
> namespace scope, per C++ [lex.name]/3.2, you can't do that like this; you'll 
> need to look at the `DeclContext` of the declaration instead of just the 
> identifier.
> If you're trying to detect whether the name is being introduced in the global 
> namespace scope,
That's indeed the goal

> you'll need to look at the DeclContext of the declaration instead of just the 
> identifier.



Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+

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.



Comment at: clang/lib/Sema/SemaDecl.cpp:16296
   } else {
+if (TUK == TUK_Definition)
+  warnOnReservedIdentifier(New);

rsmith wrote:
> Why do we not diagnose the other possible `TagUseKind`s? `struct _foo;` and 
> `struct _foo *p;` both use reserved identifiers too.
We have a test case for `struct _foo` and its correctly diagnosed. I'll double 
check for pointer / reference too.


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

[PATCH] D93095: Introduce -Wreserved-identifier

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

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(),
+  Sema::LookupAnyName,
+  Sema::ForExternalRedeclaration);
+IdentifierLookup.suppressDiagnostics();
+if (LookupName(IdentifierLookup, TUScope))

aaron.ballman wrote:
> rsmith wrote:
> > I don't understand why name lookup is involved here. Whether an identifier 
> > is reserved doesn't depend on whether it's already been declared in the 
> > global namespace or not. It's valid to declare `_foo` in some user-defined 
> > namespace regardless of whether there's already a `_foo` in the global 
> > namespace, and it's invalid to declare `_foo` in the global namespace 
> > regardless of whether there's already a `_foo` in the global namespace.
> > 
> > If you're trying to detect whether the name is being introduced in the 
> > global namespace scope, per C++ [lex.name]/3.2, you can't do that like 
> > this; you'll need to look at the `DeclContext` of the declaration instead 
> > of just the identifier.
> Sorry @serge-sans-paille for my think-o, Richard is right.
> 
> My thinking was "if the declaration can be found at the global namespace, 
> then it's reserved", but.. that's a hypothetical declaration, not an actual 
> one, so of course the name lookup won't find it. Derp.
> 
> I guess we really do have to look at the `DeclContext`.
> 
> @rsmith -- just to be clear, from http://eel.is/c++draft/lex.name#3.2, does 
> "for use as a name in the global namespace" imply macros as well? e.g., can 
> an implementation decide to use this reservation to `#define _foo 12`, which 
> would cause problems with a `_foo` declared in a user-defined namespace? If 
> so, then we can ignore what the declaration context is because the name could 
> be snatched out from under the user via a macro anyway.
Regarding macros, I think it depends what you mean.

The implementation is permitted to declare additional `_foo` names in the 
global namespace, and I think the intent is that these can be either 
predeclared or declared in headers. The user is permitted to declare `_foo` 
names in other (non-reserved) namespaces.

As a result, neither party should define an `_foo` macro, because doing so 
would step on the other party's namespace (though it would be OK for user code 
to do so after all of their standard library includes, I suppose). 
Specifically, I think it would be non-conforming for the implementation to 
`#define` such a macro (because the name is not reserved to the implementation 
in all contexts), and I think it would be UB via [macro.names]/1 for user code 
to `#define` such a macro that actually collided with a name that happened to 
be declared in the global namespace by a standard library header.

We can probably warn on `#define _foo` if we want to, but I don't think we can 
produce a `-pedantic-error` for that case, because I think it's (sometimes) 
valid for user code to do that.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D93095/new/

https://reviews.llvm.org/D93095

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__reserved' is a reserved identifier}

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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(),
+  Sema::LookupAnyName,
+  Sema::ForExternalRedeclaration);
+IdentifierLookup.suppressDiagnostics();
+if (LookupName(IdentifierLookup, TUScope))

rsmith wrote:
> I don't understand why name lookup is involved here. Whether an identifier is 
> reserved doesn't depend on whether it's already been declared in the global 
> namespace or not. It's valid to declare `_foo` in some user-defined namespace 
> regardless of whether there's already a `_foo` in the global namespace, and 
> it's invalid to declare `_foo` in the global namespace regardless of whether 
> there's already a `_foo` in the global namespace.
> 
> If you're trying to detect whether the name is being introduced in the global 
> namespace scope, per C++ [lex.name]/3.2, you can't do that like this; you'll 
> need to look at the `DeclContext` of the declaration instead of just the 
> identifier.
Sorry @serge-sans-paille for my think-o, Richard is right.

My thinking was "if the declaration can be found at the global namespace, then 
it's reserved", but.. that's a hypothetical declaration, not an actual one, so 
of course the name lookup won't find it. Derp.

I guess we really do have to look at the `DeclContext`.

@rsmith -- just to be clear, from http://eel.is/c++draft/lex.name#3.2, does 
"for use as a name in the global namespace" imply macros as well? e.g., can an 
implementation decide to use this reservation to `#define _foo 12`, which would 
cause problems with a `_foo` declared in a user-defined namespace? If so, then 
we can ignore what the declaration context is because the name could be 
snatched out from under the user via a macro anyway.


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

[PATCH] D93095: Introduce -Wreserved-identifier

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

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(),
+  Sema::LookupAnyName,
+  Sema::ForExternalRedeclaration);
+IdentifierLookup.suppressDiagnostics();
+if (LookupName(IdentifierLookup, TUScope))

I don't understand why name lookup is involved here. Whether an identifier is 
reserved doesn't depend on whether it's already been declared in the global 
namespace or not. It's valid to declare `_foo` in some user-defined namespace 
regardless of whether there's already a `_foo` in the global namespace, and 
it's invalid to declare `_foo` in the global namespace regardless of whether 
there's already a `_foo` in the global namespace.

If you're trying to detect whether the name is being introduced in the global 
namespace scope, per C++ [lex.name]/3.2, you can't do that like this; you'll 
need to look at the `DeclContext` of the declaration instead of just the 
identifier.



Comment at: clang/lib/Sema/SemaDecl.cpp:5898
 
+  if (D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_extern)
+warnOnReservedIdentifier(New);

Why does the presence / absence of `extern` matter here?



Comment at: clang/lib/Sema/SemaDecl.cpp:13640
 
+  warnOnReservedIdentifier(New);
+

Is there somewhere more central you can do this, rather than repeating it once 
for each kind of declaration? (Eg, `PushOnScopeChains`)



Comment at: clang/lib/Sema/SemaDecl.cpp:16296
   } else {
+if (TUK == TUK_Definition)
+  warnOnReservedIdentifier(New);

Why do we not diagnose the other possible `TagUseKind`s? `struct _foo;` and 
`struct _foo *p;` both use reserved identifiers too.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D93095

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // expected-warning {{'_barbouille' is a reserved identifier}}
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // no-warning
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+// we skip this one because it's not at top-level.
+int _barbatruc; // expected-warning {{'_barbatruc' is a reserved identifier}}
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__re

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 the name will be exposed at the 
> top level, I wonder if we should use `Sema::LookupName()` to try to find the 
> name in the global scope. That would move this code from `AST` into `Sema` to 
> avoid layering issues, so it'd be something more like `bool 
> Sema::IsDeclReserved(const NamedDecl *ND) const` or `bool 
> Sema::IsIdentifierReserved(const IdentifierInfo *II) const`.
> 
> The idea is to perform the lookup in global scope and if the lookup finds the 
> identifier, then regardless of how it got there (anonymous union field, 
> anonymous namespaces, enumeration constants, etc), we know it may conflict 
> with an external declaration and diagnose.
> 
> WDYT?
That's an interesting proposal.Let me try that!


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

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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 the 
top level, I wonder if we should use `Sema::LookupName()` to try to find the 
name in the global scope. That would move this code from `AST` into `Sema` to 
avoid layering issues, so it'd be something more like `bool 
Sema::IsDeclReserved(const NamedDecl *ND) const` or `bool 
Sema::IsIdentifierReserved(const IdentifierInfo *II) const`.

The idea is to perform the lookup in global scope and if the lookup finds the 
identifier, then regardless of how it got there (anonymous union field, 
anonymous namespaces, enumeration constants, etc), we know it may conflict with 
an external declaration and diagnose.

WDYT?



Comment at: clang/test/Sema/reserved-identifier.cpp:40
+  return foo__bar(); // no-warning
+}

hubert.reinterpretcast wrote:
> serge-sans-paille wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > You should also have some tests for:
> > > > ```
> > > > template 
> > > > void _Foobar(); // Even though it's not instantiated, it's still 
> > > > reserved.
> > > > 
> > > > template  // Reserved
> > > > void whatever();
> > > > 
> > > > void func() {
> > > >   int array[10];
> > > >   for (auto _A : array) // Reserved
> > > > ;
> > > > }
> > > > 
> > > > class _C { // Reserved
> > > > public:
> > > >   _C(); // Not reserved
> > > > };
> > > > 
> > > > unsigned operator "" huttah(unsigned long long); // Reserved 
> > > > (http://eel.is/c++draft/usrlit.suffix#1)
> > > > 
> > > > unsigned operator "" _W(unsigned long long); // Reserved
> > > > unsigned operator "" _w(unsigned long long); // Reserved
> > > > 
> > > > static unsigned operator "" _X(unsigned long long); // Not reserved
> > > > static unsigned operator "" _x(unsigned long long); // Not reserved
> > > > ```
> > > I think some of these tests are still missing. I'm especially worried 
> > > about the user-defined literal cases being diagnosed, as we'd be warning 
> > > to not do the exact thing users are expected to do.
> > User defined literal tested below and behave as expected.
> @aaron.ballman, the "not reserved" comment re: `_X` for the literal operator 
> using the `operator string-literal identifier` form above seems suspect to 
> me. See `_Bq` example in [over.literal].
Thanks, @hubert.reinterpretcast, you're correct -- I forgot just how hard we 
made the rules for UDLs when we decided to require a leading underscore. :-(


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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:
  clang/include/clang/AST/Decl.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // expected-warning {{'_barbeFleurie' is a reserved identifier}}
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+// we skip this one because it's not at top-level.
+int _barbatruc; // expected-warning {{'_barbatruc' is a reserved identifier}}
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reser

[PATCH] D93095: Introduce -Wreserved-identifier

[Arthur O'Dwyer via Phabricator via cfe-commits]

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."
https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier
http://eel.is/c++draft/lex.name#3.1
If the implementation predefines `#define _BarbeBleue 42` (which the 
implementation is permitted to do), then this code won't compile.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[Hubert Tong via Phabricator via cfe-commits]

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:
> > > ```
> > > template 
> > > void _Foobar(); // Even though it's not instantiated, it's still reserved.
> > > 
> > > template  // Reserved
> > > void whatever();
> > > 
> > > void func() {
> > >   int array[10];
> > >   for (auto _A : array) // Reserved
> > > ;
> > > }
> > > 
> > > class _C { // Reserved
> > > public:
> > >   _C(); // Not reserved
> > > };
> > > 
> > > unsigned operator "" huttah(unsigned long long); // Reserved 
> > > (http://eel.is/c++draft/usrlit.suffix#1)
> > > 
> > > unsigned operator "" _W(unsigned long long); // Reserved
> > > unsigned operator "" _w(unsigned long long); // Reserved
> > > 
> > > static unsigned operator "" _X(unsigned long long); // Not reserved
> > > static unsigned operator "" _x(unsigned long long); // Not reserved
> > > ```
> > I think some of these tests are still missing. I'm especially worried about 
> > the user-defined literal cases being diagnosed, as we'd be warning to not 
> > do the exact thing users are expected to do.
> User defined literal tested below and behave as expected.
@aaron.ballman, the "not reserved" comment re: `_X` for the literal operator 
using the `operator string-literal identifier` form above seems suspect to me. 
See `_Bq` example in [over.literal].


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 is another case that I'm on the fence about failing to warn on because 
> the name `_barbatruc` would conflict with a name introduced by the 
> implementation. Another interesting variant of the same problem, for C++:
> ```
> static union {
>   int _field;
> };
> ```
> I wonder if the rule should not be whether the declaration is at file scope 
> or not, but whether the declared identifier can be found at file scope or not?
> whether the declared identifier can be found at file scope or not?

100% agree. As

```
static union {
  int _field;
};
int _field;
```

is invalid, I considered that one and tested it.



Comment at: clang/test/Sema/reserved-identifier.cpp:40
+  return foo__bar(); // no-warning
+}

aaron.ballman wrote:
> aaron.ballman wrote:
> > You should also have some tests for:
> > ```
> > template 
> > void _Foobar(); // Even though it's not instantiated, it's still reserved.
> > 
> > template  // Reserved
> > void whatever();
> > 
> > void func() {
> >   int array[10];
> >   for (auto _A : array) // Reserved
> > ;
> > }
> > 
> > class _C { // Reserved
> > public:
> >   _C(); // Not reserved
> > };
> > 
> > unsigned operator "" huttah(unsigned long long); // Reserved 
> > (http://eel.is/c++draft/usrlit.suffix#1)
> > 
> > unsigned operator "" _W(unsigned long long); // Reserved
> > unsigned operator "" _w(unsigned long long); // Reserved
> > 
> > static unsigned operator "" _X(unsigned long long); // Not reserved
> > static unsigned operator "" _x(unsigned long long); // Not reserved
> > ```
> I think some of these tests are still missing. I'm especially worried about 
> the user-defined literal cases being diagnosed, as we'd be warning to not do 
> the exact thing users are expected to do.
User defined literal tested below and behave as expected.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other,   // expected-warning {{'_Other' is a reserved identifier}}
+  _other// no-warning
+};
+
+enum _Menu { // expected-warning {{'_Menu' is a reserved identifier}}
+  _OtheR_,   // expected-warning {{'_OtheR_' is a reserved identifier}}
+  _other_// expected-warning {{'_other_' is a reserved identifier}}
+};
+
+enum {
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // expected-warning {{'_other' is a reserved identifier}}
+};
+
+static union {
+  int _barbeFleurie; // expected-warning {{'_barbeFleurie' is a reserved identifier}}
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+// we skip this one because it's not at top-level.
+int _barbatruc; // expected-warning {{'_barbatruc' is a reserved identifier}}
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__re

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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 -Wreserved-identifier

Quuxplusone wrote:
> aaron.ballman wrote:
> > Can you add a test that we do not warn on an extern declaration? e.g.,
> > ```
> > extern char *_strdup(const char *);
> > ```
> > This is sometimes used (esp in older C code bases) to avoid having to 
> > include an entire header to scrape one declaration out of it, and there are 
> > popular libraries (like the MSVC CRT) that have APIs starting with a single 
> > underscore and lowercase letter.
> > 
> > The idea here is: if it's an extern declaration, the identifier is "owned" 
> > by some other declaration and this is not likely something the user has 
> > control over.
> Should that logic also apply to a forward declaration like `struct _foo;`? 
> Should it apply to `struct _foo { int i; };`? These are also things the user 
> might not have control over. (And they're equally things that the user 
> //could// pull out into a separate .h file surrounded by disabled-warning 
> pragmas, if they really wanted to.)
I'd say 100% yeah to the forward declaration, but I'm unsure about the actual 
definition, because there's no way to distinguish it from a user definition.



Comment at: clang/test/Sema/reserved-identifier.c:24
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // no-warning
+};

aaron.ballman wrote:
> I'm on the fence about whether this should have no warning or not. 
> Enumeration constants in C (and sometimes in C++, depending on the 
> enumeration) are in the enclosing scope. e.g.,
> ```
> enum foo {
>   _bar
> };
> 
> int i = _bar;
> ```
> So if a user has an enumeration constant named `_bar`, the implementation is 
> not free to add `int _bar(void);` as it will cause compile errors. WDYT?
We could state it that way: would the code still compile if there's a `int 
_bar` symbol defined in a system header. here the answer is no, so reserved.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other// expected-warning {{'_Other' is a reserved identifier}}
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+// we skip this one because it's not at top-level.
+int _barbatruc; // no-warning
+}
+
+long double operator"" _BarbeBleue(long double) // no-warning
+{
+  return 0.;
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__reserved' is a reserved identifier}}
+;
+}
+
+enum __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // no-warning
+};
+
+struct __babar { // expected-warning {{'__babar' is a reserved identifier}}
+};
+
+typedef struct {
+  int _Field; // expected-warning {{'_Field' is a reserved identifier}}
+  int _field; // no-warning
+} _Typedef;   // expected-warning {{'_Typedef' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+struct _reserved { /

[PATCH] D93095: Introduce -Wreserved-identifier

[Arthur O'Dwyer via Phabricator via cfe-commits]

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 contexts the compiler diagnoses `_X` and `_x` equally, and in 
other contexts the compiler diagnoses `_X` but fails to diagnose `_x`.

You should make two diagnostics: `-Wreserved-extern-identifier` and 
`-Wreserved-pp-identifier` (or something along those lines), and their messages 
should be carefully worded to explain //why// the warning is happening — 
"identifier %0 is reserved for use at file scope," "identifier %0 is reserved 
in all contexts," etc.

Btw, the reason some identifiers (like `_X`) are reserved in all contexts is so 
that the implementation can define them as macros (e.g. header guards).



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

aaron.ballman wrote:
> Can you add a test that we do not warn on an extern declaration? e.g.,
> ```
> extern char *_strdup(const char *);
> ```
> This is sometimes used (esp in older C code bases) to avoid having to include 
> an entire header to scrape one declaration out of it, and there are popular 
> libraries (like the MSVC CRT) that have APIs starting with a single 
> underscore and lowercase letter.
> 
> The idea here is: if it's an extern declaration, the identifier is "owned" by 
> some other declaration and this is not likely something the user has control 
> over.
Should that logic also apply to a forward declaration like `struct _foo;`? 
Should it apply to `struct _foo { int i; };`? These are also things the user 
might not have control over. (And they're equally things that the user 
//could// pull out into a separate .h file surrounded by disabled-warning 
pragmas, if they really wanted to.)


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

[PATCH] D93095: Introduce -Wreserved-identifier

[Aaron Ballman via Phabricator via cfe-commits]

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 that we do not warn on an extern declaration? e.g.,
```
extern char *_strdup(const char *);
```
This is sometimes used (esp in older C code bases) to avoid having to include 
an entire header to scrape one declaration out of it, and there are popular 
libraries (like the MSVC CRT) that have APIs starting with a single underscore 
and lowercase letter.

The idea here is: if it's an extern declaration, the identifier is "owned" by 
some other declaration and this is not likely something the user has control 
over.



Comment at: clang/test/Sema/reserved-identifier.c:24
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // no-warning
+};

I'm on the fence about whether this should have no warning or not. Enumeration 
constants in C (and sometimes in C++, depending on the enumeration) are in the 
enclosing scope. e.g.,
```
enum foo {
  _bar
};

int i = _bar;
```
So if a user has an enumeration constant named `_bar`, the implementation is 
not free to add `int _bar(void);` as it will cause compile errors. WDYT?



Comment at: clang/test/Sema/reserved-identifier.cpp:58
+// we skip this one because it's not at top-level.
+int _barbatruc; // no-warning
+}

This is another case that I'm on the fence about failing to warn on because the 
name `_barbatruc` would conflict with a name introduced by the implementation. 
Another interesting variant of the same problem, for C++:
```
static union {
  int _field;
};
```
I wonder if the rule should not be whether the declaration is at file scope or 
not, but whether the declared identifier can be found at file scope or not?



Comment at: clang/test/Sema/reserved-identifier.cpp:40
+  return foo__bar(); // no-warning
+}

aaron.ballman wrote:
> You should also have some tests for:
> ```
> template 
> void _Foobar(); // Even though it's not instantiated, it's still reserved.
> 
> template  // Reserved
> void whatever();
> 
> void func() {
>   int array[10];
>   for (auto _A : array) // Reserved
> ;
> }
> 
> class _C { // Reserved
> public:
>   _C(); // Not reserved
> };
> 
> unsigned operator "" huttah(unsigned long long); // Reserved 
> (http://eel.is/c++draft/usrlit.suffix#1)
> 
> unsigned operator "" _W(unsigned long long); // Reserved
> unsigned operator "" _w(unsigned long long); // Reserved
> 
> static unsigned operator "" _X(unsigned long long); // Not reserved
> static unsigned operator "" _x(unsigned long long); // Not reserved
> ```
I think some of these tests are still missing. I'm especially worried about the 
user-defined literal cases being diagnosed, as we'd be warning to not do the 
exact thing users are expected to do.


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

[PATCH] D93095: Introduce -Wreserved-identifier

[serge via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Sema/reserved-identifier.c
  clang/test/Sema/reserved-identifier.cpp

Index: clang/test/Sema/reserved-identifier.cpp
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }// expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _barbouille() { return 0; } // expected-warning {{'_barbouille' is a reserved identifier}}
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+template  constexpr bool __toucan = true; // expected-warning {{'__toucan' is a reserved identifier}}
+
+template 
+concept _Barbotine = __toucan; // expected-warning {{'_Barbotine' is a reserved identifier}}
+
+template  // expected-warning {{'__' is a reserved identifier}}
+struct BarbeNoire {};
+
+template  // expected-warning {{'__' is a reserved identifier}}
+void BarbeRousse() {}
+
+namespace _Barbidur { // expected-warning {{'_Barbidur' is a reserved identifier}}
+
+struct __barbidou {}; // expected-warning {{'__barbidou' is a reserved identifier}}
+struct _barbidou {};  // no-warning
+
+int __barbouille; // expected-warning {{'__barbouille' is a reserved identifier}}
+int _barbouille;  // no-warning
+
+int __babar() { return 0; } // expected-warning {{'__babar' is a reserved identifier}}
+int _babar() { return 0; }  // no-warning
+
+} // namespace _Barbidur
+
+class __barbapapa { // expected-warning {{'__barbapapa' is a reserved identifier}}
+  void _barbabelle() {} // no-warning
+  int _Barbalala;   // expected-warning {{'_Barbalala' is a reserved identifier}}
+};
+
+enum class __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some,   // expected-warning {{'__some' is a reserved identifier}}
+  _Other// expected-warning {{'_Other' is a reserved identifier}}
+};
+
+using _Barbamama = __barbapapa; // expected-warning {{'_Barbamama' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+namespace {
+// we skip this one because it's not at top-level.
+int _barbatruc; // no-warning
+}
Index: clang/test/Sema/reserved-identifier.c
===
--- /dev/null
+++ clang/test/Sema/reserved-identifier.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier -Wno-visibility %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }// no-warning
+static int _bar() { return 0; } // expected-warning {{'_bar' is a reserved identifier}}
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }// expected-warning {{'_foo' is a reserved identifier}}
+
+// This one is explicitly skipped by -Wreserved-identifier
+void *_; // no-warning
+
+void foo(unsigned int _Reserved) { // expected-warning {{'_Reserved' is a reserved identifier}}
+  unsigned int __1 =   // expected-warning {{'__1' is a reserved identifier}}
+  _Reserved;   // no-warning
+  goto __reserved;
+__reserved: // expected-warning {{'__reserved' is a reserved identifier}}
+;
+}
+
+enum __menu { // expected-warning {{'__menu' is a reserved identifier}}
+  __some, // expected-warning {{'__some' is a reserved identifier}}
+  _Other, // expected-warning {{'_Other' is a reserved identifier}}
+  _other  // no-warning
+};
+
+struct __babar { // expected-warning {{'__babar' is a reserved identifier}}
+};
+
+typedef struct {
+  int _Field; // expected-warning {{'_Field' is a reserved identifier}}
+  int _field; // no-warning
+} _Typedef;   // expected-warning {{'_Typedef' is a reserved identifier}}
+
+int foobar() {
+  return foo__bar(); // no-warning
+}
+
+struct _reserved { // expected-warning {{'_reserved' is a reserved identifier}}
+  int a;
+} cunf(void) {
+  return