[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