[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