[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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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