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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 21 11:48:19 PDT 2023


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



More information about the cfe-commits mailing list