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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
