[PATCH] D148505: Allow `__attribute__((warn_unused))` on individual constructors

[Erich Keane via Phabricator via cfe-commits]

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