aaron.ballman 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];
----------------
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.)
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
