aaron.ballman added a comment. In D129835#3655548 <https://reviews.llvm.org/D129835#3655548>, @cjdb wrote:
> In D129835#3655487 <https://reviews.llvm.org/D129835#3655487>, @aaron.ballman > wrote: > >>> When we have a function that can have its value discarded, we can use >>> [[clang::discardable]] to indicate that the `[[nodiscard]]` attribute >>> should be ignored. >> >> Alternatively, you can scope the pragma and declarations appropriately and >> then we don't need to add a new attribute. Is there a reason that wouldn't >> work for you? > > A key problem here (IMO) is that this requires manually pushing and popping > the pragma as it is required. That's a feature to me, not a bug. It means you have to actually think about what you're doing instead of being surprised. This is the same reason our community standards want you to keep anonymous namespaces as small as possible -- it's too easy to have absolutely no idea about the behavior when you're in the middle of a block of code and the relevant part is off-screen somewhere. > With `[[discardable]]` one just needs to push/pop at the extremes of a file > (and if a future version of module maps supports global pragmas for a module, > there too, but that's a discussion that requires a design doc). I understood that, I just don't think that's a good thing. This is basically an attribute that says "I know we said we wanted everything here to be nodiscard, but JUST KIDDING not this one!" which is not a very clean approach to writing headers. > It's also good for documenting "hey you can discard this thing in my sea of > nodiscard interfaces" on the interface itself. That coupling is also good for > correctness because it means something can't accidentally slip in/out of the > pragma's scope. I'd like to see some examples of that. In my experience, attributes are most often hidden behind macros which are sometimes then combined with other macros, etc, so whether the attribute is effective or not is already not always obvious from the interface; I don't see how this will move the needle. >> (This attribute would be interesting to consider if we added a feature flag >> like `-fdiagnose-discarded-results` where the default is as-if everything >> was declared `nodiscard` and then you could mark the APIs with >> `[[clang::discardable]]` for the few whose results don't matter. However, >> that's also a much bigger feature because system headers are a challenge.) > > The primary reason for me wanting to add this attribute comes from my libc++ > days, where I wanted to mark whatever made sense as nodiscard. @ldionne was > in favour in principle, but against in practice; noting that nodiscard would > clutter the interface too much, and it'd be better if the situation were > reversed (which is what this patch does). I think adding this is still a good > step for getting us to a world where `-fdiagnose-discarded-results` can be a > real consideration, especially if libc++ and llvm-libc adopt it. Yes, I wish the default were correct as well, but it's not and it won't ever be corrected for practicality reasons. I don't know what libc++'s goals are in terms of other compilers, but it seems like using a clang-specific pragma would be making it more difficult to use libc++ with any other compiler. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129835/new/ https://reviews.llvm.org/D129835 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
