[PATCH] D102122: Support warn_unused_result on typedefs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.)

> 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