erichkeane added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2996
let Spellings = [GCC<"warn_unused">];
- let Subjects = SubjectList<[Record]>;
+ let Subjects = SubjectList<[Record, CXXConstructor]>;
let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> sberg wrote:
> > aaron.ballman wrote:
> > > I'm confused -- if you want this to behave like `nodiscard`, why aren't
> > > these changes for `warn_unused_result` (which is what implements
> > > `nodiscard`)? I don't think this should go on `warn_unused` because
> > > that's for the declaration of the type as a unit and not written on
> > > functions.
> > >
> > > I think you don't need any changes to Attr.td for this functionality.
> > `[[nodiscard]]` and `__attribute__((warn_unused))` already have different
> > semantics when applied to a class: While both warn about unused expression
> > results (`-Wunused-value`), only the latter warns about unused variables
> > (`-Wunused-variable`). This patch keeps that difference, just extends it
> > to individual constructors:
> > ```
> > $ cat test.cc
> > struct [[nodiscard]] S1 {
> > S1(int) {}
> > S1(double) {}
> > };
> > struct S2 {
> > [[nodiscard]] S2(int) {}
> > S2(double) {}
> > };
> > struct __attribute__((warn_unused)) S3 {
> > S3(int) {}
> > S3(double) {}
> > };
> > struct S4 {
> > __attribute__((warn_unused)) S4(int) {}
> > S4(double) {}
> > };
> > int main() {
> > S1(0); // expected-warning {{ignoring temporary}}
> > S1(0.0); // expected-warning {{ignoring temporary}}
> > S2(0); // expected-warning {{ignoring temporary}}
> > S2(0.0);
> > S3(0); // expected-warning {{expression result unused}}
> > S3(0.0); // expected-warning {{expression result unused}}
> > S4(0); // expected-warning {{expression result unused}}
> > S4(0.0);
> > S1 s1a(0);
> > S1 s1b(0.0);
> > S2 s2a(0);
> > S2 s2b(0.0);
> > S3 s3a(0); // expected-warning {{unused variable}}
> > S3 s3b(0.0); // expected-warning {{unused variable}}
> > S4 s4a(0); // expected-warning {{unused variable}}
> > S4 s4b(0.0);
> > }
> > ```
> > ```
> > $ clang++ -Wunused-value -Wunused-variable -fsyntax-only -Xclang -verify
> > test.cc
> > ```
> Hmmm, well, that's compelling and now I don't know what I think.
>
> I'm worried that this further muddies the water between `warn_unused` and
> `warn_unused_result` which are already pretty murky to begin with.
> `warn_unused_result` goes on functions, `warn_unused` goes on the declaration
> of a type. That symmetry is something I think people can understand and
> remember despite the attribute names. With this patch, now it's
> `warn_unused_result` goes on functions, and `warn_unused` goes on constructor
> functions or the declaration of a type. It's not the worst thing ever
> (constructors don't really have "results" for example). But the fact that
> these two attributes already confuse users (AND we've got `unused` as an
> attribute in the mix as well, which doesn't help) means we should proceed
> with caution (and likely coordinate with GCC given that all three of these
> attributes came from their implementation).
>
> I'm not certain what the best design approach is yet, but I'll think about
> the problem a bit more over the next few days to see if my thoughts
> crystalize on this.
>
> (FWIW, if we do decide to go with `warn_unused`, I think it's no longer
> defensible to leave this attribute as undocumented in Clang -- we should take
> the opportunity to push up some documentation for this one.)
To double-down on what Aaron says: He and I discussed this extensively offline,
and we are both really quite unsure what to do here. I can make a
reasonable/good argument to convince myself and Aaron in either direction (and
he's been able to convince me both ways!), so I also think the ergonomics of
these two needs to be defined/fleshed out for this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148505/new/
https://reviews.llvm.org/D148505
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
