[llvm] r284364 - ADT: Use LLVM_NODISCARD instead of LLVM_ATTRIBUTE_UNUSED_RESULT for StringRef

[Justin Bogner via llvm-commits]

Zachary Turner <zturner at google.com> writes:
> On Sun, Nov 27, 2016 at 8:36 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
>>
>> > On Nov 27, 2016, at 6:24 PM, Justin Bogner via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>
>> > Similarly, what functions return a `const char *` where you
>> > don't care about the return value? I feel like the vast majority of
>> > functions that return either a `StringRef` or a `const char *` expect
>> > you to use the return value, and if you're ignoring it you probably
>> > either called the wrong function or are doing something weird (like
>> > calling an accessor function for side effects) and having to add a void
>> > cast will make that clearer.
>> >
>> > I feel pretty strongly that having nodiscard on StringRef makes for
>> > overall better code, so I'm certainly opposed to removing it like
>> > r287586 does. What real world code motivates this?
>>
>> That’s a legitimate question!
>>
>
> Tomorrow I can find the code that was triggering this warning which is what
> led me to it in the first place, but I think this is really missing the
> point.
>
> We are a library, we cannot make any assumptions about how users of our
> library use our support classes.  They are intended to be generic and serve
> arbitrary use cases, some of which are downstream and which we cannot
> possibly know about.

Maybe not, but we can certainly make contracts about how we expect them
to be used. Regardless, to the extent that we're vending StringRef as
part of the library here it's as part of return values from the LLVM
library - I don't think we're in the business of providing general
programming utilities tailored to uses outside of LLVM.

> To impose what we consider good or bad API design on someone else's
> product is not something we should be in the business of, and we
> certainly should not be doing it via compiler errors (e.g. if someone
> is using -Werror) if someone chooses an API design that we deem "bad".

I don't really see that we're imposing anything here - if someone wants
to ignore an llvm::StringRef they can cast a return value to void, which
makes their intent explicit. If an API provides a value that is ignored
often enough for this to be an annoyance maybe we should provide two
variants.

> I'm not taking a position on whether or not LLVM_NODISCARD on StringRef
> leads to overall better code, I'm only arguing that it should not be used
> at a class level except in cases where a return value must be checked **by
> design**.  StringRef is not such a class.  Neither are ArrayRef and a
> couple of other classes which have also had this attribute applied.
>
> If we're going to put it on StringRef, then we need to justify why it's not
> on APFloat or Optional<T> or PointerIntPair in the name of consistency.
> What makes StringRef fundamentally different than pretty much any other
> type anywhere with regards to when it should be checked as a return value?

Personally, I'd put it on `int` if I could. I don't see a problem with
adding nodiscard to any of those types.