Introduce -Wreserved-identifier

[serge-sans-paille]

Summary

Warn when a declaration uses an identifier that doesn't obey the reserved
identifier rule from C and/or C++.

Do not perform the check at preprocessor level because:

1. it will be quite costly
2. we don't have accurate linkage info at that point

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

376

Removing the quotes here because of a suggested edit later (the diagnostics engine will add the quotes for you).

clang/lib/AST/Decl.cpp

1094

Why the SC_None case?

1099

1104

clang/lib/Sema/SemaDecl.cpp

5548

clang/lib/Sema/SemaStmt.cpp

546

clang/test/Sema/reserved-identifier.c

32

You should add an explicit test for _ by itself.

clang/test/Sema/reserved-identifier.cpp

22

You should have a test like this but within the global namespace (which should diagnose).

30

+1 to this request.

40

You should also have some tests for:

template <typename T>
void _Foobar(); // Even though it's not instantiated, it's still reserved.

template <typename _Ty> // 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

[serge-sans-paille]

Take review into account and add more tests

[serge-sans-paille]

clang/lib/AST/Decl.cpp

1094

It happens when no explicit linkage is provided.

clang/test/Sema/reserved-identifier.cpp

30

I probably missed a lot of declarations, but I've hardened that part to handle template parameters and concepts

[aaron.ballman]

clang/include/clang/AST/Decl.h

360

Still missing the trailing full stop on the comment.

clang/include/clang/Sema/Sema.h

2466

The usual convention in the project is west const rather than east const, FWIW.

clang/lib/AST/Decl.cpp

1087

1092

This reference no longer exists. I think you want [lex.name]p3

1094

Actually, I think the real question is: why check the storage class at all? e.g.,

static int _this_is_still_reserved_at_file_scope;

The reservation is for use at file scope (or the global namespace in C++ parlance) -- it would make a good test case to add.

1103

"followed by an uppercase letter or another underscore"

also, the (2.11) is ambiguous.

clang/test/Sema/reserved-identifier.c

38

Another awful test case:

struct _reserved { int a; } func(void) { // Should diagnose
  return (struct _reserved){1};
}

void func(struct _reserved { int a; } r) {} // Should not diagnose

[logan-5]

clang/lib/AST/Decl.cpp

1094

I think @aaron.ballman is right; there was some earlier confusion (that I think I added to, apologies) over the rules for _these since there seems to be some conflicting information online (e.g. the cppreference page for the C rules appears to be wrong), but rereading both C and C++ standards carefully, I believe _these are reserved at file scope / in the global namespace in both languages, whether static or not.

[serge-sans-paille]

Take review into account, some extra tests suggested by @aaron.ballman and the according modification in Decl.cpp

[aaron.ballman]

Adding Richard for another interpreter of the standards.

clang/lib/AST/Decl.cpp

1096

The comment should probably explain why we do this rather than check if the decl context is the translation unit.

One worry I have with this approach is that there are more declaration contexts than what you've got handled here, like ObjCContainers, linkage specifications, etc.

I think "for use in the global namespace" may be a bit of a red herring here. Consider user code like:

namespace my_awesome_stuff {
// Not at global namespace.
unsigned _w() { return 0; }
}

unsigned func() {
  using namespace my_awesome_stuff;
  return _w();
}

If an implementation (or a future version of the standard) adds int _w = 12; to the global namespace, the user's code becomes ambiguous and fails to compile.

Another example of this would be if the implementation adds a macro. e.g.,

// If this gets defined by the implementation...
#define _strdup strdup

// ... it could break this user code due to duplicate symbols.
char *_strdup(const char *str);

@rsmith -- what's your feeling on this?

[serge-sans-paille]

Instead of pretending to be smart, just consider that if the considered decl is at the top level, then it's a top-level decl, otherwise it's not. There may be some interpretation wrt. the standard, but this seems both simple and conservative to me. It gies a few false negative, but it's a warning here, so that should be fine.

[serge-sans-paille]

Rebased on main.

[aaron.ballman]

clang/test/Sema/reserved-identifier.c

10

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.

25

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?

clang/test/Sema/reserved-identifier.cpp

40

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.

59

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?

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

377

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

clang/test/Sema/reserved-identifier.c

10

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

[serge-sans-paille]

Address some of the review

[serge-sans-paille]

clang/test/Sema/reserved-identifier.c

10

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.

25

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.

[serge-sans-paille]

Update codebase and testbed to reflect recent discussion.

[serge-sans-paille]

clang/test/Sema/reserved-identifier.cpp

40

User defined literal tested below and behave as expected.

59

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.

[hubert.reinterpretcast]

clang/test/Sema/reserved-identifier.cpp

40

@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].

[• Quuxplusone]

clang/test/Sema/reserved-identifier.cpp

78

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.

[serge-sans-paille]

Ignore forward declaration of tagdecl

[aaron.ballman]

clang/lib/AST/Decl.cpp

1099

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?

clang/test/Sema/reserved-identifier.cpp

40

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

[serge-sans-paille]

clang/lib/AST/Decl.cpp

1099

That's an interesting proposal.Let me try that!

[serge-sans-paille]

As suggested by @aaron.ballman base the detection of top-level-ness on Sema::LookupName to avoid re-implementing the wheel.

[rsmith]

clang/lib/Sema/Sema.cpp

2421–2428 ↗

(On Diff #317677)

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.

clang/lib/Sema/SemaDecl.cpp

5896

Why does the presence / absence of extern matter here?

13632

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

16288

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

[aaron.ballman]

clang/lib/Sema/Sema.cpp

2421–2428 ↗

(On Diff #317677)

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.

[serge-sans-paille]

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.

[rsmith]

clang/lib/Sema/Sema.cpp

2421–2428 ↗

(On Diff #317677)

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.

[serge-sans-paille]

clang/lib/Sema/Sema.cpp

2421–2428 ↗

(On Diff #317677)

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.

clang/lib/Sema/SemaDecl.cpp

13632

That would be sane. I'll check that.

16288

We have a test case for struct _foo and its correctly diagnosed. I'll double check for pointer / reference too.

[serge-sans-paille]

Extra test cases

[serge-sans-paille]

@rsmith I did my bet to address your comments. What do you think of current state?

clang/lib/Sema/SemaDecl.cpp

13632

I tried PushOnScopeChains, and this does not capture all the required declarations. I failed to find another place :-/

16288

Test case added for pointers, works like a charm.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

377

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.

clang/lib/AST/Decl.cpp

1096

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

clang/lib/Sema/SemaDecl.cpp

5547

clang/lib/Sema/SemaDeclCXX.cpp

10015

Unrelated changes?

clang/lib/Sema/SemaStmt.cpp

544

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

clang/test/Sema/reserved-identifier.c

10

Should that logic also apply to a forward declaration like struct _foo;?

I could see a case for not warning on a forward declaration like that, but I think it's a bit less compelling because I can't seem to find that pattern happening in the wild like I can for extern function declarations.

Should it apply to struct _foo { int i; };?

This is a definition of the type and so I think it should be warned on (unless it's in a system header).

50–51

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

54

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

clang/test/Sema/reserved-identifier.cpp

78

+1, this really is reserved.

[rsmith]

clang/lib/AST/Decl.cpp

1087

1096

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.

clang/lib/Sema/SemaDecl.cpp

5550–5551

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.

13632

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.

17101

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.

clang/lib/Sema/SemaTemplate.cpp

1680–1681 ↗

(On Diff #320212)

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.

[rsmith]

clang/test/Sema/reserved-identifier.c

14

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

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

32

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

struct _Zebulon3 *p;

(Nor for TUK_Friend.)

50–51

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

54

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

clang/test/Sema/reserved-identifier.cpp

22

Please also test the _not_reserved case here. Parameters are weird, because they're parsed before we have a DeclContext created to correspond with their scope, so they start off with the translation unit as their parent.

64

This should warn: this declaration would conflict with an int _barbeFleurie(); in a system header.

... although weirdly Clang doesn't diagnose that conflict. Hm. That looks like a bug.

[mizvekov]

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

[aaron.ballman]

clang/test/Sema/reserved-identifier.c

50–51

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

54

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.

[serge-sans-paille]

Patch rebased on main.

[serge-sans-paille]

Updated error message to report the reason why an identifier is reserved.

[serge-sans-paille]

Fix some formatting
Support literal operator

[aaron.ballman]

clang/include/clang/AST/Decl.h

82

Because this is not a scoped enum, should these enumerators get an RIR_ prefix added to them?

361–362

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

clang/include/clang/Basic/DiagnosticGroups.td

772

Were you planning to use this new diagnostic group?

clang/lib/AST/Decl.cpp

1084

Might as well address the lint warning.

1104–1105

[serge-sans-paille]

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

[serge-sans-paille]

clang/include/clang/AST/Decl.h

82

I used a class enum with a stream operator instead. Not a big fan of prefixed enum ;-)

clang/test/Sema/reserved-identifier.c

54

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.

[serge-sans-paille]

Fix typo

[aaron.ballman]

clang/include/clang/AST/Decl.h

82

Totally fine by me, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td

377

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 .

clang/lib/AST/Decl.cpp

1105–1111

1118–1120

1125–1127

clang/lib/Sema/SemaDecl.cpp

5547

Still missing a full stop at the end of the comment here.

5550–5551

Richard's comment is still unaddressed as well.

13632

I'm still wondering about this as well.

clang/lib/Sema/SemaStmt.cpp

545

This check should be reversed as well.

[serge-sans-paille]

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.

[serge-sans-paille]

clang/lib/Sema/SemaDecl.cpp

13632

I'll investigate that one more deeply, I do share your concern.

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

377

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

clang/lib/AST/Decl.cpp

1084

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.

1097

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

1113–1119

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
}

[serge-sans-paille]

Another round of review :-) I addressed both the scattering of warnOnReservedIdentifier and the code split between Decl.cpp and IdentifierInfo.cpp.

[serge-sans-paille]

clang/lib/AST/Decl.cpp

1097

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

[rsmith]

clang/lib/AST/Decl.cpp

1097

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.

[serge-sans-paille]

Do not use lexical parent, as suggested by @rsmith
Add test case for extern function worward-declared in function body, as suggested by @rsmith

[serge-sans-paille]

Warn on friend functions. I failed to support friend classes, but they are only declared and not defined, so that should be fine, right?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

377

Ooh, I like that suggestion, thanks!

clang/lib/AST/Decl.cpp

1084

It looks like this code still needs to move.

1096

This comment should be updated -- we no longer walk the lexical parents.

clang/lib/Basic/IdentifierTable.cpp

297 ↗

(On Diff #334662)

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.

clang/lib/CodeGen/CGDebugInfo.cpp

4051–4052 ↗

(On Diff #334662)

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

clang/lib/Sema/SemaDecl.cpp

5551–5552

clang/lib/Sema/SemaDeclCXX.cpp

10988

Spurious whitespace change.

12665

Spurious whitespace change.

16729

Doesn't PushOnScopeChains() do this already?

clang/lib/Sema/SemaStmt.cpp

545

This still needs to be reversed so we check for system headers after checking the reserved status.

clang/lib/Sema/SemaTemplate.cpp

1680–1681 ↗

(On Diff #320212)

Is this handled by PushOnScopeChains()?

[rsmith]

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

clang/include/clang/AST/Decl.h

81–82

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.

clang/include/clang/Basic/DiagnosticGroups.td

772

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.

clang/lib/CodeGen/CGDebugInfo.cpp

4051–4052 ↗

(On Diff #334662)

Should we instead be asking whether CalleeDecl has a reserved name, now that we can?

clang/lib/Sema/SemaCodeComplete.cpp

746 ↗

(On Diff #334662)

Should we be using ND->isReserved here?

clang/lib/Sema/SemaDecl.cpp

5551–5552

I'm surprised we don't warn in user headers. Is that intentional?

[serge-sans-paille]

Take into account latest reviews

[aaron.ballman]

Aside from the formatting nit and the question about the diagnostic, I think this LGTM!

clang/include/clang/Basic/DiagnosticGroups.td

606

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

clang/lib/AST/Decl.cpp

1084

The formatting issue is worth fixing.

clang/include/clang/AST/Decl.h

 class TemplateArgumentList;
 class TemplateArgumentListInfo;
 class TemplateParameterList;
 class TypeAliasTemplateDecl;
 class TypeLoc;
 class UnresolvedSetImpl;
 class VarTemplateDecl;
 
 /// The top declaration context.
 class TranslationUnitDecl : public Decl, public DeclContext {
   ASTContext &Ctx;
 
   /// The (most recently entered) anonymous namespace for this
   /// translation unit, if one has been created.
   NamespaceDecl *AnonymousNamespace = nullptr;
 
   explicit TranslationUnitDecl(ASTContext &ctx);
 
bool isCXXClassMember() const {
 
     return DC->isRecord();
   }
 
   /// Determine whether the given declaration is an instance member of
   /// a C++ class.
   bool isCXXInstanceMember() const;
 
+  /// Determine if the declaration obeys the reserved identifier rules of the
+  /// given Language
+  bool isReserved(const LangOptions &) const;
+
   /// Determine what kind of linkage this entity has.
   ///
   /// This is not the linkage as defined by the standard or the codegen notion
   /// of linkage. It is just an implementation detail that is used to compute
   /// those.
   Linkage getLinkageInternal() const;
 
   /// Get the linkage from a semantic point of view. Entities in

clang/include/clang/Basic/DiagnosticGroups.td

 
 def Unsequenced : DiagGroup<"unsequenced">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
 
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
 def KeywordAsMacro : DiagGroup<"keyword-macro">;
 def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">;
 
 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;
 def : DiagGroup<"strict-aliasing=1">;
 def : DiagGroup<"strict-aliasing=2">;
 def : DiagGroup<"strict-aliasing">;
 
 // Just silence warnings about -Wstrict-overflow for now.

 //        not just deprecated) or not?
 def GCCWriteStrings : DiagGroup<"write-strings" , [WritableStrings]>;
 
 def CharSubscript : DiagGroup<"char-subscripts">;
 def LargeByValueCopy : DiagGroup<"large-by-value-copy">;
 def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
 def SignedEnumBitfield : DiagGroup<"signed-enum-bitfield">;
 
+
+def ReservedIdentifier : DiagGroup<"reserved-identifier", [ReservedIdAsMacro]>;
+
 // Unreachable code warning groups.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at
 //  least actively used, with more noisy versions of the warning covered
 //  under separate flags.
 //
 def UnreachableCodeLoopIncrement : DiagGroup<"unreachable-code-loop-increment">;
 def UnreachableCode : DiagGroup<"unreachable-code",

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_unneeded_member_function : Warning<
   "member function %0 is not needed and will not be emitted">,
   InGroup<UnneededMemberFunction>, DefaultIgnore;
 def warn_unused_private_field: Warning<"private field %0 is not used">,
   InGroup<UnusedPrivateField>, DefaultIgnore;
 def warn_unused_lambda_capture: Warning<"lambda capture %0 is not "
   "%select{used|required to be captured for this use}1">,
   InGroup<UnusedLambdaCapture>, DefaultIgnore;
 
+def warn_reserved_identifier: Warning<
+  "'%0' is a reserved identifier">,
+  InGroup<ReservedIdentifier>, DefaultIgnore;
+
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_size: Warning<
   "return value of %0 is a large (%1 bytes) pass-by-value object; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_udt: Warning<
   "%0 has C-linkage specified, but returns user-defined type %1 which is "

clang/include/clang/Sema/Sema.h

bool mightBeIntendedToBeTemplateName(ExprResult E, bool &Dependent) {
     // Any additional cases recognized here should also be handled by
     // diagnoseExprIntendedAsTemplateName.
     return false;
   }
   void diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
                                           SourceLocation Less,
                                           SourceLocation Greater);
 
+  void warnOnReservedIdentifier(NamedDecl *D);
+
   Decl *ActOnDeclarator(Scope *S, Declarator &D);
 
   NamedDecl *HandleDeclarator(Scope *S, Declarator &D,
                               MultiTemplateParamsArg TemplateParameterLists);
   void RegisterLocallyScopedExternCDecl(NamedDecl *ND, Scope *S);
   bool DiagnoseClassNameShadow(DeclContext *DC, DeclarationNameInfo Info);
   bool diagnoseQualifiedDeclaration(CXXScopeSpec &SS, DeclContext *DC,
                                     DeclarationName Name, SourceLocation Loc,

clang/lib/AST/Decl.cpp

if (!hasCachedLinkage())
     return true;
 
   Linkage L = LinkageComputer{}
                   .computeLVForDecl(this, LVComputationKind::forLinkageOnly())
                   .getLinkage();
   return L == getCachedLinkage();
 }
 
+bool NamedDecl::isReserved(const LangOptions &LangOpts) const {
+  StringRef Name = getName();
+  // '_' is a valid identifier
+  if (Name.size() <= 1)
+    return false;
+
+  // 17.4.3.1.2 Global names [lib.global.names]
+  if (Name[0] == '_') {
+
+    // Each name that begins with an underscore is reserved to the
+    // implementation for use as a name in the global namespace
+    if (getDeclContext()->isTranslationUnit()) {
+      StorageClass SC = getStorageClass(this);
+      if (SC == SC_Extern || SC == SC_PrivateExtern || SC == SC_None)
+        return true;
+    }
+
+    // Each name that begins
+    // with an underscore followed by an uppercase letter (2.11) is reserved
+    if (Name[1] == '_' || ('A' <= Name[1] && Name[1] <= 'Z'))
+      return true;
+  }
+
+  // Each name that contains a double underscore (__)
+  if (LangOpts.CPlusPlus && Name.contains("__"))
+    return true;
+
+  return false;
+}
+
 ObjCStringFormatFamily NamedDecl::getObjCFStringFormattingFamily() const {
   StringRef name = getName();
   if (name.empty()) return SFF_None;
 
   if (name.front() == 'C')
     if (name == "CFStringCreateWithFormat" ||
         name == "CFStringCreateWithFormatAndArguments" ||
         name == "CFStringAppendFormat" ||
         name == "CFStringAppendFormatAndArguments")
       return SFF_CFString;
   return SFF_None;
 }
 
 Linkage NamedDecl::getLinkageInternal() const {
   // We don't care about visibility here, so ask for the cheapest
   // possible visibility analysis.
   return LinkageComputer{}
       .getLVForDecl(this, LVComputationKind::forLinkageOnly())
       .getLinkage();
 }
 
 LinkageInfo NamedDecl::getLinkageAndVisibility() const {
   return LinkageComputer{}.getDeclLinkageAndVisibility(this);
 }
 

clang/lib/Sema/SemaDecl.cpp

for (unsigned I = 0, E = D.getNumTypeObjects(); I != E; ++I) {
     CXXScopeSpec &SS = Chunk.Mem.Scope();
     if (S.RebuildNestedNameSpecifierInCurrentInstantiation(SS))
       return true;
   }
 
   return false;
 }
 
+void Sema::warnOnReservedIdentifier(NamedDecl *D) {
+  if (!Context.getSourceManager().isInSystemHeader(D->getLocation()) &&
+      D->isReserved(getLangOpts()))
+    Diag(D->getLocation(), diag::warn_reserved_identifier) << D->getName();
+}
+
 Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
   D.setFunctionDefinitionKind(FunctionDefinitionKind::Declaration);
   Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());
 
   if (OriginalLexicalContext && OriginalLexicalContext->isObjCContainer() &&
       Dcl && Dcl->getDeclContext()->isFileContext())
     Dcl->setTopLevelDeclInObjCContainer();
 
   if (getLangOpts().OpenCL)
     setCurrentOpenCLExtensionForDecl(Dcl);
NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   // If this has an identifier and is not a function template specialization,
   // add it to the scope stack.
   if (New->getDeclName() && AddToScope)
     PushOnScopeChains(New, S);
 
   if (isInOpenMPDeclareTargetContext())
     checkDeclIsAllowedInOpenMPTarget(nullptr, New);
 
+  warnOnReservedIdentifier(New);
   return New;
 }
 
 /// Helper method to turn variable array types into constant array
 /// types in certain situations which would otherwise be errors (for
 /// GCC compatibility).
 static QualType TryToFixInvalidVariablyModifiedType(QualType T,
                                                     ASTContext &Context,
Decl *Sema::ActOnParamDeclarator(Scope *S, Declarator &D) {
 
   if (New->hasAttr<BlocksAttr>()) {
     Diag(New->getLocation(), diag::err_block_on_nonlocal);
   }
 
   if (getLangOpts().OpenCL)
     deduceOpenCLAddressSpace(New);
 
+  warnOnReservedIdentifier(New);
+
   return New;
 }
 
 /// Synthesizes a variable for a parameter arising from a
 /// typedef.
 ParmVarDecl *Sema::BuildParmVarDeclForTypedef(DeclContext *DC,
                                               SourceLocation Loc,
                                               QualType T) {
CreateNewDecl:
   if (Invalid && getLangOpts().CPlusPlus) {
     if (New->isBeingDefined())
       if (auto RD = dyn_cast<RecordDecl>(New))
         RD->completeDefinition();
     return nullptr;
   } else if (SkipBody && SkipBody->ShouldSkip) {
     return SkipBody->Previous;
   } else {
+    warnOnReservedIdentifier(New);
     return New;
   }
 }
 
 void Sema::ActOnTagStartDefinition(Scope *S, Decl *TagD) {
   AdjustDeclIfTemplate(TagD);
   TagDecl *Tag = cast<TagDecl>(TagD);
 
void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
 
   // Verify that all the fields are okay.
   SmallVector<FieldDecl*, 32> RecFields;
 
   for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
        i != end; ++i) {
     FieldDecl *FD = cast<FieldDecl>(*i);
 
+    warnOnReservedIdentifier(FD);
+
     // Get the type for the field.
     const Type *FDTy = FD->getType().getTypePtr();
 
     if (!FD->isAnonymousStructOrUnion()) {
       // Remember all fields written by the user.
       RecFields.push_back(FD);
     }
 
Decl *Sema::ActOnEnumConstant(Scope *S, Decl *theEnumDecl, Decl *lastEnumConst,
   AddPragmaAttributes(S, New);
 
   // Register this decl in the current scope stack.
   New->setAccess(TheEnumDecl->getAccess());
   PushOnScopeChains(New, S);
 
   ActOnDocumentableDecl(New);
 
+  warnOnReservedIdentifier(New);
+
   return New;
 }
 
 // Returns true when the enum initial expression does not trigger the
 // duplicate enum warning.  A few common cases are exempted as follows:
 // Element2 = Element1
 // Element2 = Element1 + 1
 // Element2 = Element1 - 1

clang/lib/Sema/SemaDeclCXX.cpp

Sema::ActOnReenterTemplateScope(Decl *D,
   AdjustDeclIfTemplate(D);
 
   // In order to get name lookup right, reenter template scopes in order from
   // outermost to innermost.
   SmallVector<TemplateParameterList *, 4> ParameterLists;
   DeclContext *LookupDC = dyn_cast<DeclContext>(D);
 
   if (DeclaratorDecl *DD = dyn_cast<DeclaratorDecl>(D)) {
     for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i)
       ParameterLists.push_back(DD->getTemplateParameterList(i));
 
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
       if (FunctionTemplateDecl *FTD = FD->getDescribedFunctionTemplate())
         ParameterLists.push_back(FTD->getTemplateParameters());
     } else if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
       LookupDC = VD->getDeclContext();
 
Decl *Sema::ActOnStartNamespaceDef(
   ActOnDocumentableDecl(Namespc);
 
   // Although we could have an invalid decl (i.e. the namespace name is a
   // redefinition), push it as current DeclContext and try to continue parsing.
   // FIXME: We should be able to push Namespc here, so that the each DeclContext
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  warnOnReservedIdentifier(Namespc);
+
   return Namespc;
 }
 
 /// getNamespaceDecl - Returns the namespace a decl represents. If the decl
 /// is a namespace alias, returns the namespace it points to.
 static inline NamespaceDecl *getNamespaceDecl(NamedDecl *D) {
   if (NamespaceAliasDecl *AD = dyn_cast_or_null<NamespaceAliasDecl>(D))
     return AD->getNamespace();
if (auto *TD = dyn_cast_or_null<TagDecl>(DeclFromDeclSpec)) {
       handleTagNumbering(TD, S);
     }
     ActOnTypedefNameDecl(S, CurContext, NewTD, Previous, Redeclaration);
     NewND = NewTD;
   }
 
   PushOnScopeChains(NewND, S);
   ActOnDocumentableDecl(NewND);
+
+  warnOnReservedIdentifier(NewND);
+
   return NewND;
 }
 
 Decl *Sema::ActOnNamespaceAliasDef(Scope *S, SourceLocation NamespaceLoc,
                                    SourceLocation AliasLoc,
                                    IdentifierInfo *Alias, CXXScopeSpec &SS,
                                    SourceLocation IdentLoc,
                                    IdentifierInfo *Ident) {
if (ND->isInvalidDecl()) {
     if (FD->getNumTemplateParameterLists() && SS.isValid()) {
       Diag(FD->getLocation(), diag::warn_template_qualified_friend_unsupported)
         << SS.getScopeRep() << SS.getRange()
         << cast<CXXRecordDecl>(CurContext);
       FrD->setUnsupportedFriend(true);
     }
   }
 
   return ND;
 }
 
 void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   AdjustDeclIfTemplate(Dcl);
 
   FunctionDecl *Fn = dyn_cast_or_null<FunctionDecl>(Dcl);
   if (!Fn) {
     Diag(DelLoc, diag::err_deleted_non_function);

clang/lib/Sema/SemaStmt.cpp

Sema::ActOnLabelStmt(SourceLocation IdentLoc, LabelDecl *TheDecl,
                      SourceLocation ColonLoc, Stmt *SubStmt) {
   // If the label was multiply defined, reject it now.
   if (TheDecl->getStmt()) {
     Diag(IdentLoc, diag::err_redefinition_of_label) << TheDecl->getDeclName();
     Diag(TheDecl->getLocation(), diag::note_previous_definition);
     return SubStmt;
   }
 
+  if (!Context.getSourceManager().isInSystemHeader(IdentLoc) &&
+      TheDecl->isReserved(getLangOpts()))
+    Diag(IdentLoc, diag::warn_reserved_identifier) << TheDecl->getName();
+
   // Otherwise, things are good.  Fill in the declaration and return it.
   LabelStmt *LS = new (Context) LabelStmt(IdentLoc, TheDecl, SubStmt);
   TheDecl->setStmt(LS);
   if (!TheDecl->isGnuLocal()) {
     TheDecl->setLocStart(IdentLoc);
     if (!TheDecl->isMSAsmLabel()) {
       // Don't update the location of MS ASM labels.  These will result in
       // a diagnostic, and changing the location here will mess that up.

clang/test/Sema/reserved-identifier.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier %s
+
+#define __oof foo__ // expected-warning {{macro name is a reserved identifier}}
+
+int foo__bar() { return 0; }    // no-warning
+static int _bar() { return 0; } // no-warning
+static int _Bar() { return 0; } // expected-warning {{'_Bar' is a reserved identifier}}
+int _foo() { return 0; }        // expected-warning {{'_foo' 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
+  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
+}

clang/test/Sema/reserved-identifier.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wreserved-identifier %s
+
+int foo__bar() { return 0; }    // expected-warning {{'foo__bar' is a reserved identifier}}
+static int _bar() { return 0; } // no-warning
+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
+}
+
+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
+}