aaron.ballman added a comment. Thank you for the ping, this fell entirely off my radar!
In D102122#3344317 <https://reviews.llvm.org/D102122#3344317>, @dblaikie wrote: > I don't recall all the context, but did try discussing this with the > committee folks and got a variety of strong opinions/wasn't sure whether > there was a path forward: > https://lists.isocpp.org/ext/2021/05/index.php#msg16554 (for those with > access to that). What's your take on the discussion there? Worth pushing > forward? The C++ side was definitely mixed review, so I'm on the fence about whether it's worth pushing forward in C++. However, I think the story in C is rather different because there are less reasonable alternatives there and the type system is far less complex. I think it's reasonable to experiment with support in C only under `[[clang::warn_unused_result]]`, keeping in mind some of the WG21 feedback in mind regarding questions like behavior of typedefs of typedef types, etc, if that's of interest to you. The story with `[[nodiscard]]` is a bit more complex -- I would expect this attribute to appear in header files shared between C and C++, so I think it would be bad for it to be applicable to typedefs in only one language but not the other. However, so long as we issue a pedantic warning that the `[[nodiscard]]` attribute on a typedef is a Clang extension, I think we could technically do it. But it might not be worth it given the lack of portability; at that point, the user can use `[[clang::warn_unused_result]]` to the same effect. > & some minor questions I guess I wrote last year... > > In D102122#2748529 <https://reviews.llvm.org/D102122#2748529>, @aaron.ballman > wrote: > >> In D102122#2748271 <https://reviews.llvm.org/D102122#2748271>, @dblaikie >> wrote: >> >>> In D102122#2748206 <https://reviews.llvm.org/D102122#2748206>, >>> @aaron.ballman wrote: >>> >>>> 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. >>> >>> FWIW, looks like we already diverge from GCC's behavior - GCC doesn't seem >>> to support this attribute on types at all: https://godbolt.org/z/8YjqnE4cv >>> (but does support [[nodiscard]] in that place) >> >> Whomp whomp! :-( >> >>>> `[[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). >>> >>> Might be a compatible extension, though - to use a standard attribute in a >>> non-standard context? (at least clang and gcc only warn on putting >>> [[nodiscard]] in non-standard places, they don't error) >> >> It's a bit unclear -- there's a list of things the attribute applies to, and >> typedef is not on the list, so it would be reasonable to think that means >> the attribute can't be written on anything else. But because the standard >> doesn't say what happens if you DO apply it to a typedef, perhaps that's >> sufficiently undefined to allow us to call it a conforming extension. Given >> that you're fine trying to get this standardized and it seems like it >> shouldn't be contentious, I think we aren't behaving badly if we accept >> `[[nodiscard]]` on a typedef so long as we give an extension warning. > > Fair enough - does the spec say what happens if you use a completely unknown > spelling like `[[foobar]]`? I guess the spec reserves that for future > attributes, so implementations aren't meant to add support in there > (implementations being expected to use some namespace to put their extensions > in) > > Just curiious. It's allowed with implementation-defined semantics (per https://eel.is/c++draft/dcl.attr#grammar-6.sentence-1) but we'd be evil, bad folks and I'd be opposed to it as attribute code owner due to it stepping on the WG21 and WG14 design space for attributes (per https://eel.is/c++draft/dcl.attr#grammar-6.sentence-3). >>>> Do you have an appetite to talk to the GCC and standards folks? >>> >>> Medium interest. >>> >>>> 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. >>> >>> Happy to go either way. >>> >>> Is there an easy/recommended way to split these things up? Duplicate the >>> records in the td/come up with separate names, etc?) >> >> Given that we already diverge from GCC for `warn_unused_result`, and because >> you're willing to give the standardization bit a shot and that seems highly >> likely to succeed, I say let's try to keep the same semantic effects for all >> of the spellings in terms of what the attribute does. If that's reasonable >> to everyone, then in SemaDeclAttr.cpp, when we see the standard spelling on >> a typedef declaration (`using` as well as `typedef`), we can issue a Clang >> extension warning on that particular use so that it's clear this is not yet >> standardized behavior for that spelling. > > Sure enough - I'll add a warning. The advice here is somewhat different on the question of spellings, now that we have more information from WG21. >> Btw, if you ever find yourself needing to distinguish between various >> spellings for the semantics of an attribute, you can use an `Accessors` list >> in the .td file (e.g., >> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L665). >> >>>> 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. >>> >>> Hmm - what if we end up with different behavior for the different spellings >>> of the attribute (as GCC does)? Can we check them separately? >> >> `__has_[c|cpp]_attribute()` has you specify the full name of the attribute >> (including vendor namespace), so we can check them separately depending on >> the spelling. So we are able to make `__has_cpp_attribute(nodiscard)` return >> a different value from `__has_cpp_attribute(clang::warn_unused_result)` >> that's different from `__has_cpp_attribute(gnu::warn_unused_result)` and >> vary the return value depending on which one has what features. One >> potential issue is that it seems we return a rather silly nonzero value for >> the clang and gnu spellings: https://godbolt.org/z/3ajxdeWr1 so feature >> testing this could be a bit awkward. :-/ > > Not sure I'm following here. If we enable the new behavior for all the > spellings - I guess we could pick a new, higher value for > `__has_cpp_attribute` to return, which would let us detect the newer > functionality? Correct. > Hoping that our high value is higher than a value GCC returns? We'd likely want to coordinate with GCC in some form or fashion rather than rely on hope, but yes, this part is a bit fragile. > & the "silly" nonzero value is silly because it's year/month, rather than > some kind of version? or year/month/day? Yeah, silly that we return the same year/month value for all spellings. The standard spelling requires that year/month format, but the vendor attributes could use a version number. Or a bitmask of supported entities you can apply the attribute to, or whatever. 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
