AaronBallman wrote:
> > > > Oh, I see, you're suggesting we remove the `getStdNamespace` check from
> > > > this PR. Yeah, I think that's reasonable.
> > >
> > >
> > > Yep, that is my suggestion, sorry I was insufficiently clear.
> > > > But I'd somewhat question whether this PR and warning really has
> > > > anything to do with the attribute names being "reserved" at that point
> > > > -- we're not checking whether they're reserved or not, and it really
> > > > doesn't matter. Warning on a `#define` that clobbers the name of a
> > > > standard attribute is just generally a good thing to do, regardless of
> > > > whether you're using the standard library.
> > >
> > >
> > > I agree with this 100%. The link to the 'reserved by the standard' is I
> > > think a good additional justification.
> > > I think the proposal, complaining about these as reserved, is a good
> > > idea/good patch. BUT I think getting caught up in the "well, when is it
> > > technically NOT UB" is a waste of time, given that the warning is a good
> > > idea even without that justification.
> >
> >
> > I think the warning is justified even without a standard library header
> > being included, but I also wonder if that means putting this under
> > `-Wreserved-identifier` is the wrong home and maybe this is a
> > `-Wattributes` warning group instead. We could reword the diagnostic to
> > something along the lines of "macro name conflicts with the name of a
> > %select{vendor attribute prefix|standard attribute|attribute name}0" and we
> > warn on all three of these cases:
> > ```
> > #define msvc 12 // conflicts with [[msvc::no_unique_address]]
> > #define annotate 12 // conflicts with [[clang::annotate]]
> > #define nodiscard 12 // conflicts with [[nodiscard]]
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > WDYT?
>
> First, I think this needs its OWN warning group, as I can see justification
> for disabling just this.
Agreed, I didn't mean to imply otherwise, I was speaking about the top-level
umbrella (sorry for the confusion).
> Second, I think that the 'standard' attribute interference is a level of
> severity HIGHER than the others thanks to being in the standard (and thus,
> perhaps, likely more to be used/interfered with).
I think the problem is the same regardless of whether the attribute is a
standard attribute or a vendor attribute -- either way, redefining the meaning
of something known to the implementation seems worth of letting the user know
about.
> Third: DOES annotate conflict with attribute annotate? Isn't that a
> function/would have to be a function macro?
Ah, good point, but the basic premise still stands.
> Fourth: I think the 'reserved name' has a level of gravitas/concreteness that
> makes the diagnostic more meaningful/immediately obvious to folks. A
> diagnostic of, "This name you chose for your macro might make this attribute
> no workie" yields "Yeah, but i wont use that so I'm ok". A diagnostic of,
> "This name is UB because the standard reserves it!" yields a level of
> pause/consideration that we otherwise wouldn't get.
Yes, but the point is: these names aren't reserved but the pain can still
happen. e.g., `cdecl` is not a reserved identifier, but re-defining its meaning
can still break code in ways that could have better diagnostics:
https://godbolt.org/z/vTfWPsYj9. Though, I suppose to reduce the chattiness, we
could elect to diagnose when we see an attribute that is ill-formed due to the
macro. e.g.,
```
#define cdecl 12
#define stdcall 100 // No warning, doesn't cause problems (yet)
void func() [[gnu::cdecl]]; // Expands to `[[gnu::12]] which is an error, so
warn on the #define above
```
> SO I think the diagnostic being associated with it being reserved is
> COMPLETELY valid/justifiable. I think "this is a bad idea, UB or not" is a
> reason to not try at all to suppress this if we think you're not ACTUALLY
> breaking the rule (by not including an StdLib header).
If we're only going to diagnose conflicts with standard attribute names, then I
can squint enough to agree. But I think that's an arbitrary limitation; we all
just agreed that this isn't about using a reserved identifier so much as about
writing a macro which will conflict with uses of attributes of the same name.
https://github.com/llvm/llvm-project/pull/106036
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
