[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
dblaikie marked an inline comment as done.
Closed by commit rGcb08f4aa4467: Support warn_unused_result on typedefs 
(authored by dblaikie).

Changed prior to commit:
  https://reviews.llvm.org/D102122?vs=433499=433878#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Basic/Attributes.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp

Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,35 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+typedef a indirect;
+a af1();
+indirect indirectf1();
+void af2() {
+  af1(); // expected-warning {{ignoring return value}}
+  void *(*a1)();
+  a1(); // no warning
+  a (*a2)();
+  a2(); // expected-warning {{ignoring return value}}
+  indirectf1(); // expected-warning {{ignoring return value}}
+}
+[[nodiscard]] typedef void *b1; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+[[gnu::warn_unused_result]] typedef void *b2; // expected-warning {{'[[gnu::warn_unused_result]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+b1 b1f1();
+b2 b2f1();
+void bf2() {
+  b1f1(); // no warning
+  b2f1(); // no warning
+}
+__attribute__((warn_unused_result)) typedef void *c;
+c cf1();
+void cf2() {
+  cf1(); // expected-warning {{ignoring return value}}
+  void *(*c1)();
+  c1();
+  c (*c2)();
+  c2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -185,7 +185,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: clang/include/clang/Basic/Attr.td:2945
C2x<"", "nodiscard", 201904>,
CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];

dblaikie wrote:
> aaron.ballman wrote:
> > Oops, it looks like we support this spelling in C++ but not in C (not your 
> > bug, just an observation): https://godbolt.org/z/1hPq6coba
> Oh, so we'd have to put `C2x<"clang", "warn_unused_result">` in here? Fair 
> enough, yeah, separate issue.
Yeah, I can take care of that one pretty quickly once your changes land (so we 
don't step on each other's toes). Or if you want to tackle it, I'm fine with 
that as well.



Comment at: clang/lib/AST/Expr.cpp:1525-1527
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;

dblaikie wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > This should loop over `TypedefType` sugar; the attribute could be in a 
> > > nested typedef.
> > I agree with Richard here. I think this is needed for a case like:
> > ```
> > typedef int foo __attribute__((warn_unused_result));
> > typedef foo foob;
> > 
> > foob func();
> > 
> > int main() {
> >   func(); // Still want to be barked at here
> > }
> > ```
> > But this example brings up an interesting question of expectations. What's 
> > your intuition on how this should behave?
> > ```
> > typedef int foo __attribute__((warn_unused_result));
> > typedef foo *foo_ptr;
> > 
> > foo_ptr func();
> > 
> > int main() {
> >   func(); // Bark? Silence? Other?
> > }
> > ```
> > FWIW, my initial inclination was that this would diagnose the call to 
> > `func()` but now I'm second-guessing that, hence the question about type 
> > composition in a typedef.
> > I agree with Richard here. I think this is needed for a case like:
> > ```
> > typedef int foo __attribute__((warn_unused_result));
> > typedef foo foob;
> > 
> > foob func();
> > 
> > int main() {
> >  func(); // Still want to be barked at here
> > }
> > ```
> 
> Oh, sorry, I marked this as "done" because I'd done this - but hadn't updated 
> the patch. Should be updated now, including a test case 
> (warn-unused-result.cpp:unused_typedef_result::indirect).
> 
> > But this example brings up an interesting question of expectations. What's 
> > your intuition on how this should behave?
> > ```
> > typedef int foo __attribute__((warn_unused_result));
> > typedef foo *foo_ptr;
> > 
> > foo_ptr func();
> > 
> > int main() {
> >   func(); // Bark? Silence? Other?
> > }
> > ```
> > FWIW, my initial inclination was that this would diagnose the call to 
> > func() but now I'm second-guessing that, hence the question about type 
> > composition in a typedef.
> 
> Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a 
> template with a warn_unused_result doesn't/shouldn't designate that template 
> specialization as warn_unused_result - it's a related/derived type, but 
> doesn't necessarily have the same properties (I guess another example in that 
> direction: a function pointer, where one of the parameters/result type are 
> warn_unused_result doesn't make the function pointer type itself 
> warn_unused_result).
> Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a 
> template with a warn_unused_result doesn't/shouldn't designate that template 
> specialization as warn_unused_result - it's a related/derived type, but 
> doesn't necessarily have the same properties (I guess another example in that 
> direction: a function pointer, where one of the parameters/result type are 
> warn_unused_result doesn't make the function pointer type itself 
> warn_unused_result).

SGTM, thanks for thinking it through with me!



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3159
 
+  if ((!AL.isGNUAttribute() && !(AL.isStandardAttributeSyntax() && 
AL.isClangScope())) && isa(D)) {
+S.Diag(AL.getLoc(), diag::warn_unused_result_typedef_unsupported_spelling) 
<< AL.isGNUScope();

This looks like it goes over the usual 80-col limit, so you should probably 
format this when landing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added a comment.

In D102122#3550590 , @aaron.ballman 
wrote:

> In D102122#3544655 , @dblaikie 
> wrote:
>
>> How's this look now, then?
>
> I think it's heading in the right direction.
>
>> One drawback is the built-in attribute warnings that mention which entities 
>> the attribute is valid on will mention typedefs even when the user uses a 
>> spelling that wouldn't be valid there - so the user might try it there, then 
>> get the follow-on warning (totally open to wordsmithing that warning, too - 
>> just placeholder words - also made it part of the same group as the 
>> warn_unused_result warning itself - if you disable that warning, then 
>> presumably you don't care about the warning being in the wrong place either, 
>> to some extent).
>
> Yeah, that is a drawback -- we don't have a spelling-specific set of subjects 
> currently. I had a plan to address that at some point by introducing more 
> data into the subject list so you could do something like:
>
>   def WarnUnusedResult : InheritableAttr {
> let Spellings = [CXX11<"", "nodiscard", 201907>,
>  C2x<"", "nodiscard", 201904>,
>  CXX11<"clang", "warn_unused_result">,
>  GCC<"warn_unused_result">];
> let Subjects = SubjectList<
>  SubjectsFor<[ObjCMethod, Enum, Record, 
> FunctionLike, TypedefName], [GNU<"warn_unused_result">, CXX11<"clang", 
> "warn_unused_result">]>,
>  SubjectsFor<[ObjCMethod, Enum, Record, 
> FunctionLike], [CXX11<"", "nodiscard">, C2x<"", "nodiscard">, CXX11<"gnu", 
> "warn_unused_result">]>,
>>;
> ...
>   }
>
> But that's complicated (as this attribute demonstrates!). This is a case 
> where we want HALF of a spelling to apply to some subjects (the GNU spelling 
> half of the `GCC` spelling) and the other half to apply to other subjects 
> (the C++ spelling half of the `GCC` spelling).
>
> I think for right now, I think it's okay for us to have the slightly degraded 
> diagnostic behavior. If it turns into user confusion in practice, we can 
> improve the diagnostics at that point. WDYT?

Oh, yeah, should've said I'm OK with it if you are. Only meant to point it out 
to get confirmation there. Thanks!




Comment at: clang/include/clang/Basic/Attr.td:2945
C2x<"", "nodiscard", 201904>,
CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];

aaron.ballman wrote:
> Oops, it looks like we support this spelling in C++ but not in C (not your 
> bug, just an observation): https://godbolt.org/z/1hPq6coba
Oh, so we'd have to put `C2x<"clang", "warn_unused_result">` in here? Fair 
enough, yeah, separate issue.



Comment at: clang/lib/AST/Expr.cpp:1525-1527
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;

aaron.ballman wrote:
> rsmith wrote:
> > This should loop over `TypedefType` sugar; the attribute could be in a 
> > nested typedef.
> I agree with Richard here. I think this is needed for a case like:
> ```
> typedef int foo __attribute__((warn_unused_result));
> typedef foo foob;
> 
> foob func();
> 
> int main() {
>   func(); // Still want to be barked at here
> }
> ```
> But this example brings up an interesting question of expectations. What's 
> your intuition on how this should behave?
> ```
> typedef int foo __attribute__((warn_unused_result));
> typedef foo *foo_ptr;
> 
> foo_ptr func();
> 
> int main() {
>   func(); // Bark? Silence? Other?
> }
> ```
> FWIW, my initial inclination was that this would diagnose the call to 
> `func()` but now I'm second-guessing that, hence the question about type 
> composition in a typedef.
> I agree with Richard here. I think this is needed for a case like:
> ```
> typedef int foo __attribute__((warn_unused_result));
> typedef foo foob;
> 
> foob func();
> 
> int main() {
>  func(); // Still want to be barked at here
> }
> ```

Oh, sorry, I marked this as "done" because I'd done this - but hadn't updated 
the patch. Should be updated now, including a test case 
(warn-unused-result.cpp:unused_typedef_result::indirect).

> But this example brings up an interesting question of expectations. What's 
> your intuition on how this should behave?
> ```
> typedef int foo __attribute__((warn_unused_result));
> typedef foo *foo_ptr;
> 
> foo_ptr func();
> 
> int main() {
>   func(); // Bark? Silence? Other?
> }
> ```
> FWIW, my initial inclination was that this would diagnose the call to func() 
> but now I'm second-guessing that, hence the question about type composition 
> in a typedef.

Yeah, I'm OK with/think that probably shouldn't warn - like instantiating a 
template with a 

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 433499.
dblaikie added a comment.

Fix/update the unsupported syntax warning


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Basic/Attributes.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp

Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,35 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+typedef a indirect;
+a af1();
+indirect indirectf1();
+void af2() {
+  af1(); // expected-warning {{ignoring return value}}
+  void *(*a1)();
+  a1(); // no warning
+  a (*a2)();
+  a2(); // expected-warning {{ignoring return value}}
+  indirectf1(); // expected-warning {{ignoring return value}}
+}
+[[nodiscard]] typedef void *b1; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+[[gnu::warn_unused_result]] typedef void *b2; // expected-warning {{'[[gnu::warn_unused_result]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+b1 b1f1();
+b2 b2f1();
+void bf2() {
+  b1f1(); // no warning
+  b2f1(); // no warning
+}
+__attribute__((warn_unused_result)) typedef void *c;
+c cf1();
+void cf2() {
+  cf1(); // expected-warning {{ignoring return value}}
+  void *(*c1)();
+  c1();
+  c (*c2)();
+  c2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -185,7 +185,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -7,4 +7,4 @@
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace 

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 433492.
dblaikie marked 4 inline comments as done.
dblaikie added a comment.

Check through multiple typedef layers to find the warn_unused_result attribute
Rephrase unsupported syntax warning


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Basic/Attributes.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp

Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,32 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+typedef a indirect;
+a af1();
+indirect indirectf1();
+void af2() {
+  af1(); // expected-warning {{ignoring return value}}
+  void *(*a1)();
+  a1(); // no warning
+  a (*a2)();
+  a2(); // expected-warning {{ignoring return value}}
+  indirectf1(); // expected-warning {{ignoring return value}}
+}
+[[nodiscard]] typedef void *b; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
+b bf1();
+void bf2() {
+  bf1(); // no warning
+}
+__attribute__((warn_unused_result)) typedef void *c;
+c cf1();
+void cf2() {
+  cf1(); // expected-warning {{ignoring return value}}
+  void *(*c1)();
+  c1();
+  c (*c2)();
+  c2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -185,7 +185,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -7,4 +7,4 @@
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+namespace 

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D102122#3544655 , @dblaikie wrote:

> How's this look now, then?

I think it's heading in the right direction.

> One drawback is the built-in attribute warnings that mention which entities 
> the attribute is valid on will mention typedefs even when the user uses a 
> spelling that wouldn't be valid there - so the user might try it there, then 
> get the follow-on warning (totally open to wordsmithing that warning, too - 
> just placeholder words - also made it part of the same group as the 
> warn_unused_result warning itself - if you disable that warning, then 
> presumably you don't care about the warning being in the wrong place either, 
> to some extent).

Yeah, that is a drawback -- we don't have a spelling-specific set of subjects 
currently. I had a plan to address that at some point by introducing more data 
into the subject list so you could do something like:

  def WarnUnusedResult : InheritableAttr {
let Spellings = [CXX11<"", "nodiscard", 201907>,
 C2x<"", "nodiscard", 201904>,
 CXX11<"clang", "warn_unused_result">,
 GCC<"warn_unused_result">];
let Subjects = SubjectList<
 SubjectsFor<[ObjCMethod, Enum, Record, 
FunctionLike, TypedefName], [GNU<"warn_unused_result">, CXX11<"clang", 
"warn_unused_result">]>,
 SubjectsFor<[ObjCMethod, Enum, Record, 
FunctionLike], [CXX11<"", "nodiscard">, C2x<"", "nodiscard">, CXX11<"gnu", 
"warn_unused_result">]>,
   >;
...
  }

But that's complicated (as this attribute demonstrates!). This is a case 
where we want HALF of a spelling to apply to some subjects (the GNU spelling 
half of the `GCC` spelling) and the other half to apply to other subjects (the 
C++ spelling half of the `GCC` spelling).

I think for right now, I think it's okay for us to have the slightly degraded 
diagnostic behavior. If it turns into user confusion in practice, we can 
improve the diagnostics at that point. WDYT?




Comment at: clang/include/clang/Basic/Attr.td:2945
C2x<"", "nodiscard", 201904>,
CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];

Oops, it looks like we support this spelling in C++ but not in C (not your bug, 
just an observation): https://godbolt.org/z/1hPq6coba



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8733-8734
+def warn_unused_result_typedef_unsupported_spelling : Warning<
+  "warn_unused_result on typedefs only supported with gnu or clang scoped "
+  "standard spelling">, InGroup;
 def warn_unused_volatile : Warning<

It's a bit more wordy, but I think it's more clear as well. Also, this makes it 
more clear that the attribute is being ignored (and groups it under that 
grouping as well).

(Definitely need to adjust for 80 col limits.)



Comment at: clang/lib/AST/Expr.cpp:1525-1527
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;

rsmith wrote:
> This should loop over `TypedefType` sugar; the attribute could be in a nested 
> typedef.
I agree with Richard here. I think this is needed for a case like:
```
typedef int foo __attribute__((warn_unused_result));
typedef foo foob;

foob func();

int main() {
  func(); // Still want to be barked at here
}
```
But this example brings up an interesting question of expectations. What's your 
intuition on how this should behave?
```
typedef int foo __attribute__((warn_unused_result));
typedef foo *foo_ptr;

foo_ptr func();

int main() {
  func(); // Bark? Silence? Other?
}
```
FWIW, my initial inclination was that this would diagnose the call to `func()` 
but now I'm second-guessing that, hence the question about type composition in 
a typedef.



Comment at: clang/test/SemaCXX/warn-unused-result.cpp:259
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+a af1();

I think you should also have a test for `[[gnu::warn_unused_result]]` to show 
that we didn't touch the behavior of that vendor-specific spelling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added a comment.

In D102122#3539812 , @aaron.ballman 
wrote:

> In D102122#3538668 , @dblaikie 
> wrote:
>
>> Sorry, with all the layers in the previous messages I'm not sure what the 
>> summary is. Sounds like the summary is "this is OK to continue work in the 
>> direction of supporting [[clang::warn_unused_result]] (only that spelling) 
>> for typedefs in C and C++ (for compatibility with C/so that C headers, like 
>> LLVM C's APIs can still be parsed as C++), while considering some of the 
>> complications raised in the WG21 discussion"?
>
> Yup! Though I agree with Richard's later comment that we could maybe also 
> extend the `__attribute__((warn_unused_result))` spelling as well.
>
>> OK, so I updated this patch with a very first draft/attempt at that - I 
>> wasn't sure how to add typedef support to only the clang spelling - so I 
>> created a new attribute name in the .td file (is there a better/more 
>> suitable way?)
>
> I would not go that route. Instead, I'd add it as a subject to the existing 
> attribute, and then add a spelling-based restriction in 
> `handleWarnUnusedResult()` to give an appropriate diagnostic if the subject 
> is wrong based on the spelling used. Something like:
>
>   if ((!AL.isGNUAttributeSyntax() || !(AL.isStandardAttributeSyntax() && 
> AL.isClangScope())) && isa(D))
> Diag(...);
>
> (Note, you may need to extend ParsedAttr somewhat, I don't recall if we have 
> all those helper functions explicitly.)

Ah, OK. Added those (& I think the first `||` was meant to be a `&&` there, 
then it seems to do what's expected)

How's this look now, then?

One drawback is the built-in attribute warnings that mention which entities the 
attribute is valid on will mention typedefs even when the user uses a spelling 
that wouldn't be valid there - so the user might try it there, then get the 
follow-on warning (totally open to wordsmithing that warning, too - just 
placeholder words - also made it part of the same group as the 
warn_unused_result warning itself - if you disable that warning, then 
presumably you don't care about the warning being in the wrong place either, to 
some extent).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 432804.
dblaikie added a comment.

Use a single attribute, with a filter in the sema parsing code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Expr.cpp
  clang/lib/Basic/Attributes.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp

Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,29 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *a;
+a af1();
+void af2() {
+  af1(); // expected-warning {{ignoring return value}}
+  void *(*a1)();
+  a1(); // no warning
+  a (*a2)();
+  a2(); // expected-warning {{ignoring return value}}
+}
+[[nodiscard]] typedef void *b; // expected-warning {{warn_unused_result on typedefs only supported with gnu or clang scoped standard spelling}}
+b bf1();
+void bf2() {
+  bf1(); // no warning
+}
+__attribute__((warn_unused_result)) typedef void *c;
+c cf1();
+void cf2() {
+  cf1(); // expected-warning {{ignoring return value}}
+  void *(*c1)();
+  c1();
+  c (*c2)();
+  c2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -185,7 +185,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -7,4 +7,4 @@
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, and function pointers}}
+namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
Index: clang/lib/Sema/SemaDeclAttr.cpp

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D102122#3538668 , @dblaikie wrote:

> Sorry, with all the layers in the previous messages I'm not sure what the 
> summary is. Sounds like the summary is "this is OK to continue work in the 
> direction of supporting [[clang::warn_unused_result]] (only that spelling) 
> for typedefs in C and C++ (for compatibility with C/so that C headers, like 
> LLVM C's APIs can still be parsed as C++), while considering some of the 
> complications raised in the WG21 discussion"?

Yup! Though I agree with Richard's later comment that we could maybe also 
extend the `__attribute__((warn_unused_result))` spelling as well.

> OK, so I updated this patch with a very first draft/attempt at that - I 
> wasn't sure how to add typedef support to only the clang spelling - so I 
> created a new attribute name in the .td file (is there a better/more suitable 
> way?)

I would not go that route. Instead, I'd add it as a subject to the existing 
attribute, and then add a spelling-based restriction in 
`handleWarnUnusedResult()` to give an appropriate diagnostic if the subject is 
wrong based on the spelling used. Something like:

  if ((!AL.isGNUAttributeSyntax() || !(AL.isStandardAttributeSyntax() && 
AL.isClangScope())) && isa(D))
Diag(...);

(Note, you may need to extend ParsedAttr somewhat, I don't recall if we have 
all those helper functions explicitly.)

> and that seems to have created one follow-on issue (partly because of my 
> other choice: Clang still only /instantiates WarnUnusedResultAttr class, not 
> the new WarnUnusedResultClangAttr class - even for the clang spelling, it 
> uses the non-clang attribute class, so /most/ of the code looking at the 
> attribute class type continues to work) - that issue is in  
> SemaCXX/cxx11-attr-print.cpp it doesn't print the attribute spelling 
> correctly, it chooses the C++ standard spelling when trying to reproduce the 
> Clang spelling, I guess because the ClangAttr class isn't used. (but of 
> course if I change the code to actually instantiate the 
> WarnUnusedResultClangAttr class - then all the existing code that handles the 
> attribute class/implements the warning has to be updated to test both 
> attribute classes... )
>
> Is there another way I should be approaching this?

I'd try to stick with just the one attribute definition in Attr.td, that should 
resolve a lot of these issues.

> (oh, and there was something about a supported version number for the `has 
> attribute` checking - where would that factor into all this?)

That's likely to get involved to address. Some of the `Spelling` objects have a 
version field you can set (like `CXX11` and `C2x` spellings); that's the value 
returned by `__has_cpp_attribute` and `__has_c_attribute`. We don't have such a 
field for the `GNU` spelling and `__has_attribute`, or the `GCC` and `Clang` 
spellings. Adding one would involve going into ClangAttrEmitter.cpp and 
updating the tablegen code to support it.

I'd say we should prove that we like the feature to ourselves, and worry about 
the version number either later in this patch or in a follow-up.




Comment at: clang/include/clang/Basic/Attr.td:2943
+def WarnUnusedResultClang : InheritableAttr {
+  let Spellings = [CXX11<"clang", "warn_unused_result">];
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, 
TypedefName]>;

rsmith wrote:
> Should we allow this new behavior for the GNU attribute spelling too? (Not 
> `[[gnu::..]]` but `__attribute__((warn_unused_result))`). We don't generally 
> avoid extending the meaning of `__attribute__((...))` compared to GCC.
I'd be okay with that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2943
+def WarnUnusedResultClang : InheritableAttr {
+  let Spellings = [CXX11<"clang", "warn_unused_result">];
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, 
TypedefName]>;

Should we allow this new behavior for the GNU attribute spelling too? (Not 
`[[gnu::..]]` but `__attribute__((warn_unused_result))`). We don't generally 
avoid extending the meaning of `__attribute__((...))` compared to GCC.



Comment at: clang/lib/AST/Expr.cpp:1525-1527
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;

This should loop over `TypedefType` sugar; the attribute could be in a nested 
typedef.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Sorry, with all the layers in the previous messages I'm not sure what the 
summary is. Sounds like the summary is "this is OK to continue work in the 
direction of supporting [[clang::warn_unused_result]] (only that spelling) for 
typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM 
C's APIs can still be parsed as C++), while considering some of the 
complications raised in the WG21 discussion"?

OK, so I updated this patch with a very first draft/attempt at that - I wasn't 
sure how to add typedef support to only the clang spelling - so I created a new 
attribute name in the .td file (is there a better/more suitable way?) and that 
seems to have created one follow-on issue (partly because of my other choice: 
Clang still only /instantiates WarnUnusedResultAttr class, not the new 
WarnUnusedResultClangAttr class - even for the clang spelling, it uses the 
non-clang attribute class, so /most/ of the code looking at the attribute class 
type continues to work) - that issue is in  SemaCXX/cxx11-attr-print.cpp it 
doesn't print the attribute spelling correctly, it chooses the C++ standard 
spelling when trying to reproduce the Clang spelling, I guess because the 
ClangAttr class isn't used. (but of course if I change the code to actually 
instantiate the WarnUnusedResultClangAttr class - then all the existing code 
that handles the attribute class/implements the warning has to be updated to 
test both attribute classes... )

Is there another way I should be approaching this?

(oh, and there was something about a supported version number for the `has 
attribute` checking - where would that factor into all this?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 432131.
dblaikie added a comment.

Separate out clang:warn_unused_result so typedef support can be added to only 
that spelling.

This has one lingering issue (because it still currently instantiates both the 
clang:warn_unused_result as the same *Attr class as the others - and so 
SemaCXX/cxx11-attr-print.cpp is failing because the AST printing for the clang 
spelling decides to use the C++ standard spelling, I guess because the 
non-clang Attr class doesn't realize it was spelled with the clang spelling)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/warn-unused-result.cpp


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' 
attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();
+  p2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -186,6 +186,7 @@
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResultClang (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, 
SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8740,6 +8740,9 @@
   case ParsedAttr::AT_WarnUnusedResult:
 handleWarnUnusedResult(S, D, AL);
 break;
+  case ParsedAttr::AT_WarnUnusedResultClang:
+handleWarnUnusedResult(S, D, AL);
+break;
   case ParsedAttr::AT_WeakRef:
 handleWeakRefAttr(S, D, AL);
 break;
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1522,6 +1522,10 @@
 if (const auto *A = TD->getAttr())
   return A;
 
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2939,10 +2939,16 @@
   let SimpleHandler = 1;
 }
 
+def WarnUnusedResultClang : InheritableAttr {
+  let Spellings = [CXX11<"clang", "warn_unused_result">];
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, 
TypedefName]>;
+  let Args = [StringArgument<"Message", 1>];
+  let Documentation = [WarnUnusedResultsDocs];
+}
+
 def WarnUnusedResult : InheritableAttr {
   let Spellings = [CXX11<"", "nodiscard", 201907>,
C2x<"", "nodiscard", 201904>,
-   CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];
   let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
   let Args = [StringArgument<"Message", 1>];


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 432091.
dblaikie added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/Expr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' 
attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();
+  p2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning 
{{'warn_unused_result' attribute only applies to Objective-C methods, enums, 
structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning 
{{'warn_unused_result' attribute only applies to Objective-C methods, enums, 
structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies 
to Objective-C methods, enums, structs, unions, classes, functions, and 
function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies 
to Objective-C methods, enums, structs, unions, classes, functions, function 
pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -185,7 +185,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, 
SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -7,4 +7,4 @@
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only 
applies to Objective-C methods, enums, structs, unions, classes, functions, and 
function pointers}}
+namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only 
applies to Objective-C methods, enums, structs, unions, classes, functions, 
function pointers, and typedefs}}
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1522,6 +1522,10 @@
 if (const auto *A = TD->getAttr())
   return A;
 
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2944,7 +2944,7 @@
C2x<"", 

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the ping, this fell entirely off my radar!

In D102122#3344317 , @dblaikie wrote:

> I don't recall all the context, but did try discussing this with the 
> committee folks and got a variety of strong opinions/wasn't sure whether 
> there was a path forward: 
> https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with 
> access to that). What's your take on the discussion there? Worth pushing 
> forward?

The C++ side was definitely mixed review, so I'm on the fence about whether 
it's worth pushing forward in C++. However, I think the story in C is rather 
different because there are less reasonable alternatives there and the type 
system is far less complex. I think it's reasonable to experiment with support 
in C only under `[[clang::warn_unused_result]]`, keeping in mind some of the 
WG21 feedback in mind regarding questions like behavior of typedefs of typedef 
types, etc, if that's of interest to you.

The story with `[[nodiscard]]` is a bit more complex -- I would expect this 
attribute to appear in header files shared between C and C++, so I think it 
would be bad for it to be applicable to typedefs in only one language but not 
the other. However, so long as we issue a pedantic warning that the 
`[[nodiscard]]` attribute on a typedef is a Clang extension, I think we could 
technically do it. But it might not be worth it given the lack of portability; 
at that point, the user can use `[[clang::warn_unused_result]]` to the same 
effect.

> & some minor questions I guess I wrote last year...
>
> In D102122#2748529 , @aaron.ballman 
> wrote:
>
>> In D102122#2748271 , @dblaikie 
>> wrote:
>>
>>> In D102122#2748206 , 
>>> @aaron.ballman wrote:
>>>
 Let me start off by saying: thanks, I think this is really useful 
 functionality. As a ridiculously obvious example, Annex K has an integer 
 type alias `errno_t` and it would incredibly handy to be able to mark that 
 as `[[nodiscard]]` to strongly encourage checking for errors for functions 
 that return that type (not that we support Annex K, but the general idea 
 extends to any integer-based error type).

 That said, there are multiple different ways to spell the same semantic 
 attribute, and it's not clear to me that we want to change them all the 
 same way.

 `[[clang::warn_unused_result]]` is ours to do with as we please, so 
 there's no issue changing that behavior.

 `__attribute__((warn_unused_result))` and `[[gnu::warn_unused_result]]` 
 are governed by GCC and we try to be compatible with their behavior. GCC 
 does not appear to support the attribute on typedefs from my testing, so 
 we'd want to coordinate with the GCC folks to see if there's an appetite 
 for the change.
>>>
>>> FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem 
>>> to support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv 
>>> (but does support [[nodiscard]] in that place)
>>
>> Whomp whomp! :-(
>>
 `[[nodiscard]]` is governed by both the C++ and C standards and we should 
 not be changing its behavior without some extra diagnostics about 
 extensions (and, preferably, some sort of attempt to standardize the 
 behavior with WG14/WG21).
>>>
>>> Might be a compatible extension, though - to use a standard attribute in a 
>>> non-standard context? (at least clang and gcc only warn on putting 
>>> [[nodiscard]] in non-standard places, they don't error)
>>
>> It's a bit unclear -- there's a list of things the attribute applies to, and 
>> typedef is not on the list, so it would be reasonable to think that means 
>> the attribute can't be written on anything else. But because the standard 
>> doesn't say what happens if you DO apply it to a typedef, perhaps that's 
>> sufficiently undefined to allow us to call it a conforming extension. Given 
>> that you're fine trying to get this standardized and it seems like it 
>> shouldn't be contentious, I think we aren't behaving badly if we accept 
>> `[[nodiscard]]` on a typedef so long as we give an extension warning.
>
> Fair enough - does the spec say what happens if you use a completely unknown 
> spelling like `[[foobar]]`? I guess the spec reserves that for future 
> attributes, so implementations aren't meant to add support in there 
> (implementations being expected to use some namespace to put their extensions 
> in)
>
> Just curiious.

It's allowed with implementation-defined semantics (per 
https://eel.is/c++draft/dcl.attr#grammar-6.sentence-1) but we'd be evil, bad 
folks and I'd be opposed to it as attribute code owner due to it stepping on 
the WG21 and WG14 design space for attributes (per 
https://eel.is/c++draft/dcl.attr#grammar-6.sentence-3).

 Do you have an 

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.
Herald added a project: All.

In D102122#3344317 , @dblaikie wrote:

> I don't recall all the context, but did try discussing this with the 
> committee folks and got a variety of strong opinions/wasn't sure whether 
> there was a path forward: 
> https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with 
> access to that). What's your take on the discussion there? Worth pushing 
> forward?

Ping on this


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I don't recall all the context, but did try discussing this with the committee 
folks and got a variety of strong opinions/wasn't sure whether there was a path 
forward: https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those 
with access to that). What's your take on the discussion there? Worth pushing 
forward?

& some minor questions I guess I wrote last year...

In D102122#2748529 , @aaron.ballman 
wrote:

> In D102122#2748271 , @dblaikie 
> wrote:
>
>> In D102122#2748206 , 
>> @aaron.ballman wrote:
>>
>>> Let me start off by saying: thanks, I think this is really useful 
>>> functionality. As a ridiculously obvious example, Annex K has an integer 
>>> type alias `errno_t` and it would incredibly handy to be able to mark that 
>>> as `[[nodiscard]]` to strongly encourage checking for errors for functions 
>>> that return that type (not that we support Annex K, but the general idea 
>>> extends to any integer-based error type).
>>>
>>> That said, there are multiple different ways to spell the same semantic 
>>> attribute, and it's not clear to me that we want to change them all the 
>>> same way.
>>>
>>> `[[clang::warn_unused_result]]` is ours to do with as we please, so there's 
>>> no issue changing that behavior.
>>>
>>> `__attribute__((warn_unused_result))` and `[[gnu::warn_unused_result]]` are 
>>> governed by GCC and we try to be compatible with their behavior. GCC does 
>>> not appear to support the attribute on typedefs from my testing, so we'd 
>>> want to coordinate with the GCC folks to see if there's an appetite for the 
>>> change.
>>
>> FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem 
>> to support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv 
>> (but does support [[nodiscard]] in that place)
>
> Whomp whomp! :-(
>
>>> `[[nodiscard]]` is governed by both the C++ and C standards and we should 
>>> not be changing its behavior without some extra diagnostics about 
>>> extensions (and, preferably, some sort of attempt to standardize the 
>>> behavior with WG14/WG21).
>>
>> Might be a compatible extension, though - to use a standard attribute in a 
>> non-standard context? (at least clang and gcc only warn on putting 
>> [[nodiscard]] in non-standard places, they don't error)
>
> It's a bit unclear -- there's a list of things the attribute applies to, and 
> typedef is not on the list, so it would be reasonable to think that means the 
> attribute can't be written on anything else. But because the standard doesn't 
> say what happens if you DO apply it to a typedef, perhaps that's sufficiently 
> undefined to allow us to call it a conforming extension. Given that you're 
> fine trying to get this standardized and it seems like it shouldn't be 
> contentious, I think we aren't behaving badly if we accept `[[nodiscard]]` on 
> a typedef so long as we give an extension warning.

Fair enough - does the spec say what happens if you use a completely unknown 
spelling like `[[foobar]]`? I guess the spec reserves that for future 
attributes, so implementations aren't meant to add support in there 
(implementations being expected to use some namespace to put their extensions 
in)

Just curiious.

>>> Do you have an appetite to talk to the GCC and standards folks?
>>
>> Medium interest.
>>
>>> If not, then I think the safest bet is to only change the behavior for the 
>>> `[[clang::warn_unused_result]]` and to leave the other forms alone for now.
>>
>> Happy to go either way.
>>
>> Is there an easy/recommended way to split these things up? Duplicate the 
>> records in the td/come up with separate names, etc?)
>
> Given that we already diverge from GCC for `warn_unused_result`, and because 
> you're willing to give the standardization bit a shot and that seems highly 
> likely to succeed, I say let's try to keep the same semantic effects for all 
> of the spellings in terms of what the attribute does. If that's reasonable to 
> everyone, then in SemaDeclAttr.cpp, when we see the standard spelling on a 
> typedef declaration (`using` as well as `typedef`), we can issue a Clang 
> extension warning on that particular use so that it's clear this is not yet 
> standardized behavior for that spelling.

Sure enough - I'll add a warning.

> Btw, if you ever find yourself needing to distinguish between various 
> spellings for the semantics of an attribute, you can use an `Accessors` list 
> in the .td file (e.g., 
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L665).
>
>>> In D102122#2746426 , @dblaikie 
>>> wrote:
>>>
 Fixes for a few other test cases (though I wonder if these tests are 
 overconstrained - do we need to be testing the list of things this 
 attribute can be applied to in so many places?)
>>>
>>> If 

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

In D102122#2748271 , @dblaikie wrote:

> In D102122#2748206 , @aaron.ballman 
> wrote:
>
>> Let me start off by saying: thanks, I think this is really useful 
>> functionality. As a ridiculously obvious example, Annex K has an integer 
>> type alias `errno_t` and it would incredibly handy to be able to mark that 
>> as `[[nodiscard]]` to strongly encourage checking for errors for functions 
>> that return that type (not that we support Annex K, but the general idea 
>> extends to any integer-based error type).
>>
>> That said, there are multiple different ways to spell the same semantic 
>> attribute, and it's not clear to me that we want to change them all the same 
>> way.
>>
>> `[[clang::warn_unused_result]]` is ours to do with as we please, so there's 
>> no issue changing that behavior.
>>
>> `__attribute__((warn_unused_result))` and `[[gnu::warn_unused_result]]` are 
>> governed by GCC and we try to be compatible with their behavior. GCC does 
>> not appear to support the attribute on typedefs from my testing, so we'd 
>> want to coordinate with the GCC folks to see if there's an appetite for the 
>> change.
>
> FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem to 
> support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv (but 
> does support [[nodiscard]] in that place)

Whomp whomp! :-(

>> `[[nodiscard]]` is governed by both the C++ and C standards and we should 
>> not be changing its behavior without some extra diagnostics about extensions 
>> (and, preferably, some sort of attempt to standardize the behavior with 
>> WG14/WG21).
>
> Might be a compatible extension, though - to use a standard attribute in a 
> non-standard context? (at least clang and gcc only warn on putting 
> [[nodiscard]] in non-standard places, they don't error)

It's a bit unclear -- there's a list of things the attribute applies to, and 
typedef is not on the list, so it would be reasonable to think that means the 
attribute can't be written on anything else. But because the standard doesn't 
say what happens if you DO apply it to a typedef, perhaps that's sufficiently 
undefined to allow us to call it a conforming extension. Given that you're fine 
trying to get this standardized and it seems like it shouldn't be contentious, 
I think we aren't behaving badly if we accept `[[nodiscard]]` on a typedef so 
long as we give an extension warning.

>> Do you have an appetite to talk to the GCC and standards folks?
>
> Medium interest.
>
>> If not, then I think the safest bet is to only change the behavior for the 
>> `[[clang::warn_unused_result]]` and to leave the other forms alone for now.
>
> Happy to go either way.
>
> Is there an easy/recommended way to split these things up? Duplicate the 
> records in the td/come up with separate names, etc?)

Given that we already diverge from GCC for `warn_unused_result`, and because 
you're willing to give the standardization bit a shot and that seems highly 
likely to succeed, I say let's try to keep the same semantic effects for all of 
the spellings in terms of what the attribute does. If that's reasonable to 
everyone, then in SemaDeclAttr.cpp, when we see the standard spelling on a 
typedef declaration (`using` as well as `typedef`), we can issue a Clang 
extension warning on that particular use so that it's clear this is not yet 
standardized behavior for that spelling.

Btw, if you ever find yourself needing to distinguish between various spellings 
for the semantics of an attribute, you can use an `Accessors` list in the .td 
file (e.g., 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L665).

>> In D102122#2746426 , @dblaikie 
>> wrote:
>>
>>> Fixes for a few other test cases (though I wonder if these tests are 
>>> overconstrained - do we need to be testing the list of things this 
>>> attribute can be applied to in so many places?)
>>
>> If the semantics of the attribute are identical regardless of how it's 
>> spelled, then we probably don't need the tests all spread out in multiple 
>> files. However, I don't think there's an intention to keep all of the 
>> spellings with identical semantics, so the different tests might still make 
>> sense (but could perhaps be cleaned up).
>>
>> In D102122#2746424 , @dblaikie 
>> wrote:
>>
>>> Oh, one question: If we do move forward with this patch, how would we 
>>> detect that the compiler supports this new use of warn_unused_result? (so 
>>> that the feature detection macros in LLVM properly make the attribute a 
>>> no-op unless we have a version of clang that supports it)
>>
>> `__has_[c|cpp]_attribute` returns a value, so we'd likely wind up using that 
>> return value to distinguish between versions.
>
> Hmm - what if 

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D102122#2748206 , @aaron.ballman 
wrote:

> Let me start off by saying: thanks, I think this is really useful 
> functionality. As a ridiculously obvious example, Annex K has an integer type 
> alias `errno_t` and it would incredibly handy to be able to mark that as 
> `[[nodiscard]]` to strongly encourage checking for errors for functions that 
> return that type (not that we support Annex K, but the general idea extends 
> to any integer-based error type).
>
> That said, there are multiple different ways to spell the same semantic 
> attribute, and it's not clear to me that we want to change them all the same 
> way.
>
> `[[clang::warn_unused_result]]` is ours to do with as we please, so there's 
> no issue changing that behavior.
>
> `__attribute__((warn_unused_result))` and `[[gnu::warn_unused_result]]` are 
> governed by GCC and we try to be compatible with their behavior. GCC does not 
> appear to support the attribute on typedefs from my testing, so we'd want to 
> coordinate with the GCC folks to see if there's an appetite for the change.

FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem to 
support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv (but 
does support [[nodiscard]] in that place)

> `[[nodiscard]]` is governed by both the C++ and C standards and we should not 
> be changing its behavior without some extra diagnostics about extensions 
> (and, preferably, some sort of attempt to standardize the behavior with 
> WG14/WG21).

Might be a compatible extension, though - to use a standard attribute in a 
non-standard context? (at least clang and gcc only warn on putting 
[[nodiscard]] in non-standard places, they don't error)

> Do you have an appetite to talk to the GCC and standards folks?

Medium interest.

> If not, then I think the safest bet is to only change the behavior for the 
> `[[clang::warn_unused_result]]` and to leave the other forms alone for now.

Happy to go either way.

Is there an easy/recommended way to split these things up? Duplicate the 
records in the td/come up with separate names, etc?)

> In D102122#2746426 , @dblaikie 
> wrote:
>
>> Fixes for a few other test cases (though I wonder if these tests are 
>> overconstrained - do we need to be testing the list of things this attribute 
>> can be applied to in so many places?)
>
> If the semantics of the attribute are identical regardless of how it's 
> spelled, then we probably don't need the tests all spread out in multiple 
> files. However, I don't think there's an intention to keep all of the 
> spellings with identical semantics, so the different tests might still make 
> sense (but could perhaps be cleaned up).
>
> In D102122#2746424 , @dblaikie 
> wrote:
>
>> Oh, one question: If we do move forward with this patch, how would we detect 
>> that the compiler supports this new use of warn_unused_result? (so that the 
>> feature detection macros in LLVM properly make the attribute a no-op unless 
>> we have a version of clang that supports it)
>
> `__has_[c|cpp]_attribute` returns a value, so we'd likely wind up using that 
> return value to distinguish between versions.

Hmm - what if we end up with different behavior for the different spellings of 
the attribute (as GCC does)? Can we check them separately?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Let me start off by saying: thanks, I think this is really useful 
functionality. As a ridiculously obvious example, Annex K has an integer type 
alias `errno_t` and it would incredibly handy to be able to mark that as 
`[[nodiscard]]` to strongly encourage checking for errors for functions that 
return that type (not that we support Annex K, but the general idea extends to 
any integer-based error type).

That said, there are multiple different ways to spell the same semantic 
attribute, and it's not clear to me that we want to change them all the same 
way.

`[[clang::warn_unused_result]]` is ours to do with as we please, so there's no 
issue changing that behavior.

`__attribute__((warn_unused_result))` and `[[gnu::warn_unused_result]]` are 
governed by GCC and we try to be compatible with their behavior. GCC does not 
appear to support the attribute on typedefs from my testing, so we'd want to 
coordinate with the GCC folks to see if there's an appetite for the change.

`[[nodiscard]]` is governed by both the C++ and C standards and we should not 
be changing its behavior without some extra diagnostics about extensions (and, 
preferably, some sort of attempt to standardize the behavior with WG14/WG21).

Do you have an appetite to talk to the GCC and standards folks? If not, then I 
think the safest bet is to only change the behavior for the 
`[[clang::warn_unused_result]]` and to leave the other forms alone for now.

In D102122#2746426 , @dblaikie wrote:

> Fixes for a few other test cases (though I wonder if these tests are 
> overconstrained - do we need to be testing the list of things this attribute 
> can be applied to in so many places?)

If the semantics of the attribute are identical regardless of how it's spelled, 
then we probably don't need the tests all spread out in multiple files. 
However, I don't think there's an intention to keep all of the spellings with 
identical semantics, so the different tests might still make sense (but could 
perhaps be cleaned up).

In D102122#2746424 , @dblaikie wrote:

> Oh, one question: If we do move forward with this patch, how would we detect 
> that the compiler supports this new use of warn_unused_result? (so that the 
> feature detection macros in LLVM properly make the attribute a no-op unless 
> we have a version of clang that supports it)

`__has_[c|cpp]_attribute` returns a value, so we'd likely wind up using that 
return value to distinguish between versions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 343885.
dblaikie added a comment.
Herald added a subscriber: jdoerfert.

Fixes for a few other test cases (though I wonder if these tests are 
overconstrained - do we need to be testing the list of things this attribute 
can be applied to in so many places?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/Expr.cpp
  clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/c2x-nodiscard.c
  clang/test/Sema/unused-expr.c
  clang/test/SemaCXX/warn-unused-result.cpp


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' 
attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();
+  p2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/test/Sema/unused-expr.c
===
--- clang/test/Sema/unused-expr.c
+++ clang/test/Sema/unused-expr.c
@@ -96,7 +96,7 @@
   return 0;
 }
 
-int t7 __attribute__ ((warn_unused_result)); // expected-warning 
{{'warn_unused_result' attribute only applies to Objective-C methods, enums, 
structs, unions, classes, functions, and function pointers}}
+int t7 __attribute__ ((warn_unused_result)); // expected-warning 
{{'warn_unused_result' attribute only applies to Objective-C methods, enums, 
structs, unions, classes, functions, function pointers, and typedefs}}
 
 // PR4010
 int (*fn4)(void) __attribute__ ((warn_unused_result));
Index: clang/test/Sema/c2x-nodiscard.c
===
--- clang/test/Sema/c2x-nodiscard.c
+++ clang/test/Sema/c2x-nodiscard.c
@@ -15,7 +15,7 @@
 [[nodiscard]] int f1(void);
 enum [[nodiscard]] E1 { One };
 
-[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies 
to Objective-C methods, enums, structs, unions, classes, functions, and 
function pointers}}
+[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies 
to Objective-C methods, enums, structs, unions, classes, functions, function 
pointers, and typedefs}}
 
 struct [[nodiscard]] S4 {
   int i;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -174,7 +174,7 @@
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
 // CHECK-NEXT: VecTypeHint (SubjectMatchRule_function)
 // CHECK-NEXT: WarnUnused (SubjectMatchRule_record)
-// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, 
SubjectMatchRule_enum, SubjectMatchRule_record, 
SubjectMatchRule_hasType_functionType, SubjectMatchRule_type_alias)
 // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, 
SubjectMatchRule_record)
 // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)
 // CHECK-NEXT: WebAssemblyExportName (SubjectMatchRule_function)
Index: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -7,4 +7,4 @@
 [[nodiscard]] int f();
 enum [[nodiscard]] E {};
 
-namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only 
applies to Objective-C methods, enums, structs, unions, classes, functions, and 
function pointers}}
+namespace [[nodiscard]] N {} // expected-warning {{'nodiscard' attribute only 
applies to Objective-C methods, enums, structs, unions, classes, functions, 
function pointers, and typedefs}}
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1413,6 +1413,10 @@
 if (const auto *A = TD->getAttr())
   return A;
 
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
Index: 

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Oh, one question: If we do move forward with this patch, how would we detect 
that the compiler supports this new use of warn_unused_result? (so that the 
feature detection macros in LLVM properly make the attribute a no-op unless we 
have a version of clang that supports it)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102122/new/

https://reviews.llvm.org/D102122

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102122: Support warn_unused_result on typedefs

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added a reviewer: aaron.ballman.
dblaikie requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

While it's not as robust as using the attribute on enums/classes (the
type information may be lost through a function pointer, a declaration
or use of the underlying type without using the typedef, etc) but I
think there's still value in being able to attribute a typedef and have
all return types written with that typedef pick up the
warn_unused_result behavior.

Specifically I'd like to be able to annotate LLVMErrorRef (a wrapper for
llvm::Error used in the C API - the underlying type is a raw pointer, so
it can't be attributed itself) to reduce the chance of unhandled errors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102122

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/AST/Expr.cpp
  clang/test/SemaCXX/warn-unused-result.cpp


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' 
attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();
+  p2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1413,6 +1413,10 @@
 if (const auto *A = TD->getAttr())
   return A;
 
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2812,7 +2812,7 @@
C2x<"", "nodiscard", 201904>,
CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];
-  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, 
TypedefName]>;
   let Args = [StringArgument<"Message", 1>];
   let Documentation = [WarnUnusedResultsDocs];
   let AdditionalMembers = [{


Index: clang/test/SemaCXX/warn-unused-result.cpp
===
--- clang/test/SemaCXX/warn-unused-result.cpp
+++ clang/test/SemaCXX/warn-unused-result.cpp
@@ -254,3 +254,15 @@
 
 void i([[nodiscard]] bool (*fp)()); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
 }
+
+namespace unused_typedef_result {
+[[clang::warn_unused_result]] typedef void *p;
+p f1();
+void f2() {
+  f1(); // expected-warning {{ignoring return value}}
+  void *(*p1)();
+  p1(); // no warning
+  p (*p2)();
+  p2(); // expected-warning {{ignoring return value}}
+}
+}
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1413,6 +1413,10 @@
 if (const auto *A = TD->getAttr())
   return A;
 
+  if (const auto *TD = getCallReturnType(Ctx)->getAs())
+if (const auto *A = TD->getDecl()->getAttr())
+  return A;
+
   // Otherwise, see if the callee is marked nodiscard and return that attribute
   // instead.
   const Decl *D = getCalleeDecl();
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2812,7 +2812,7 @@
C2x<"", "nodiscard", 201904>,
CXX11<"clang", "warn_unused_result">,
GCC<"warn_unused_result">];
-  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
+  let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, TypedefName]>;
   let Args = [StringArgument<"Message", 1>];
   let Documentation = [WarnUnusedResultsDocs];
   let AdditionalMembers = [{
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits