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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
