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
