[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<TypedefType>())
+    if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
+      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<TypedefNameDecl>(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