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

[Zachary Turner via llvm-commits]

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.  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'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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/e944a7cf/attachment.html>