[PATCH] D129835: [clang] adds a discardable attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

In D129835#4287866 <https://reviews.llvm.org/D129835#4287866>, @cjdb wrote:

>>> 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.
>
> @aaron.ballman Ugh, I've finally come up with a good use-case for `[[discardable]]`.
>
>   class [[nodiscard]] iterator {
>   public:
>     // usual iterator stuff
>   
>     [[clang::discardable]] iterator operator++(int);
>     [[clang::discardable]] iterator operator--(int);
>   };
>
> I hate it, but the alternative is to decorate everything else with `[[nodiscard]]` just to facilitate these two operators. It's a compelling use-case, but I'm not sure if it's compelling enough on its own. WDYT?
> (I personally think that those two should be nodiscard, but `i++` is pervasive enough that it might never be practical to correct.)

I'm still not super motivated because I think this encourages a confused design, because marking a *type* as `nodiscard` is making a promise about the type as a whole. That asserts there's *never* a valid time to ignore a value of the type. If you have times when that type is reasonable to ignore, then the type isn't actually non-discardable, and you should mark the methods returning the type. (IOW, "nodiscard" is part of the contract for the type in some limited ways even if it's not reflected by the type system.)

What's more, I think `iterator` is a case where it'd be a mistake to mark the type `nodiscard`, because there are plenty times when it's valid to ignore the type (consider many of the functions in `<algorithm>`, like `std::copy()`). I think it's more appropriate to mark the functions returning an iterator rather than the type itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129835/new/

https://reviews.llvm.org/D129835