[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 <https://reviews.llvm.org/D102122#3539812>, @aaron.ballman wrote:

> In D102122#3538668 <https://reviews.llvm.org/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<TypedefNameDecl>(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