This is an archive of the discontinued LLVM Phabricator instance.

Introduce -Wreserved-identifier
ClosedPublic

Authored by serge-sans-paille on Dec 11 2020, 2:03 AM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Dec 15 2020, 10:07 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
383

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
5565
clang/lib/Sema/SemaStmt.cpp
547
clang/test/Sema/reserved-identifier.c
33

You should add an explicit test for _ by itself.

clang/test/Sema/reserved-identifier.cpp
23

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

30

+1 to this request.

41

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

Take review into account and add more tests

serge-sans-paille marked 14 inline comments as done.Dec 16 2020, 7:21 AM
serge-sans-paille added inline comments.
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 added inline comments.Dec 16 2020, 8:11 AM
clang/include/clang/AST/Decl.h
360

Still missing the trailing full stop on the comment.

clang/include/clang/Sema/Sema.h
2593

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 added inline comments.Dec 16 2020, 9:46 AM
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 marked an inline comment as done.

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

aaron.ballman added a subscriber: rsmith.

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 edited the summary of this revision. (Show Details)

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.

Rebased on main.

aaron.ballman added inline comments.Jan 6 2021, 9:22 AM
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
41

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 added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

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

Address some of the review

serge-sans-paille marked an inline comment as done.Jan 8 2021, 2:16 AM
serge-sans-paille added inline comments.
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.

Update codebase and testbed to reflect recent discussion.

serge-sans-paille marked 2 inline comments as done.Jan 8 2021, 6:44 AM
serge-sans-paille added inline comments.
clang/test/Sema/reserved-identifier.cpp
41

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 added inline comments.
clang/test/Sema/reserved-identifier.cpp
41

@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 added inline comments.Jan 8 2021, 7:05 AM
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 marked 2 inline comments as done.

Ignore forward declaration of tagdecl

aaron.ballman added inline comments.Jan 8 2021, 8:11 AM
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
41

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

clang/lib/AST/Decl.cpp
1099

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

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

rsmith added inline comments.Jan 19 2021, 4:08 PM
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
5919

Why does the presence / absence of extern matter here?

13736

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

16390

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

aaron.ballman added inline comments.Jan 20 2021, 6:09 AM
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.

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 added inline comments.Jan 28 2021, 12:22 PM
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 marked an inline comment as done.Jan 28 2021, 12:53 PM
serge-sans-paille added inline comments.
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
13736

That would be sane. I'll check that.

16390

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

dmajor added a subscriber: dmajor.Jan 29 2021, 8:47 AM

Extra test cases

serge-sans-paille marked an inline comment as done.Jan 29 2021, 2:07 PM

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

clang/lib/Sema/SemaDecl.cpp
13736

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

16390

Test case added for pointers, works like a charm.

aaron.ballman added inline comments.Feb 1 2021, 9:51 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

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
5564
clang/lib/Sema/SemaDeclCXX.cpp
10078

Unrelated changes?

clang/lib/Sema/SemaStmt.cpp
545

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
clang/test/Sema/reserved-identifier.cpp
78

+1, this really is reserved.

rsmith added inline comments.Feb 3 2021, 3:42 PM
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
5567–5568

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.

13736

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.

17202

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

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 added inline comments.Feb 3 2021, 3:42 PM
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 added a subscriber: mizvekov.EditedFeb 3 2021, 4:29 PM

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 added inline comments.Feb 4 2021, 5:22 AM
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.

Patch rebased on main.

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

Fix some formatting
Support literal operator

aaron.ballman added inline comments.Mar 16 2021, 12:58 PM
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
806

Were you planning to use this new diagnostic group?

clang/lib/AST/Decl.cpp
1084

Might as well address the lint warning.

1104–1105

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 marked 10 inline comments as done.Mar 29 2021, 4:12 AM
serge-sans-paille added inline comments.
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.

aaron.ballman added inline comments.Mar 30 2021, 8:55 AM
clang/include/clang/AST/Decl.h
82

Totally fine by me, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td
384

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
5564

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

5567–5568

Richard's comment is still unaddressed as well.

13736

I'm still wondering about this as well.

clang/lib/Sema/SemaStmt.cpp
546

This check should be reversed as well.

serge-sans-paille marked 10 inline comments as done.

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.

clang/lib/Sema/SemaDecl.cpp
13736

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

rsmith added inline comments.Mar 30 2021, 4:22 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

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
}

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

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 added inline comments.Mar 31 2021, 10:48 AM
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.

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

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

serge-sans-paille marked 3 inline comments as done.Apr 1 2021, 6:00 AM
aaron.ballman added inline comments.Apr 1 2021, 7:49 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

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

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
4060–4063

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
5568–5569
clang/lib/Sema/SemaDeclCXX.cpp
11031

Spurious whitespace change.

12704

Spurious whitespace change.

16781

Doesn't PushOnScopeChains() do this already?

clang/lib/Sema/SemaStmt.cpp
546

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

clang/lib/Sema/SemaTemplate.cpp
1680–1681

Is this handled by PushOnScopeChains()?

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
806

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
4060–4063

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

clang/lib/Sema/SemaCodeComplete.cpp
743

Should we be using ND->isReserved here?

clang/lib/Sema/SemaDecl.cpp
5568–5569

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

serge-sans-paille marked 10 inline comments as done.

Take into account latest reviews

aaron.ballman accepted this revision.May 3 2021, 5:47 AM

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

clang/include/clang/Basic/DiagnosticGroups.td
641

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.

This revision is now accepted and ready to land.May 3 2021, 5:47 AM
This revision was landed with ongoing or failed builds.May 4 2021, 2:20 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 2:20 AM