erichkeane 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.
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).
Third: DOES annotate conflict with attribute annotate? Isn't that a
function/would have to be a function macro?
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.
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).
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
