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

[Zachary Turner via llvm-commits]

Adding a few other people who also agreed that nodiscard on StringRef was
overly aggressive.

StringRef is not a class which -- by design -- must be checked lest the
programmer has written erroneous code.  As a result, LLVM_NODISCARD is
fundamentally not the right tool.  For the same reason we wouldn't want
LLVM_NODISCARD on std::string or const char *.  It is far too versatile a
class.  LLVM_NODISCARD means, literally "If you dont' check this return
value, your code has a bug".

On Sun, Nov 27, 2016 at 6:24 PM Justin Bogner <mail at justinbogner.com> wrote:

> Zachary Turner <zturner at google.com> writes:
> > After some discussion on IRC, I removed this from StringRef.  It doesn't
> > seem appropriate for every function that ever returns a StringRef to
> > require having its return value checked.  One could easily imagine a
> > function like this:
> >
> > StringRef insert(StringRef S) {
> >   // compute a hash of S, store it in some hash table, then return the
> hash
> > as a StringRef.
> > }
> >
> > where someone doesn't care about the actual hash code, they just want to
> > insert it.  This is a contrived example, but in any case, this is not
> much
> > different than const char *.  Just because a function returns a const
> char*
> > doesn't mean I care about the return value.
>
> But if people don't care about the hash code, why does insert() even
> return it? 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.
>

The designer of a library cannot guarantee what a user of a library is
interested in.  A common paradigm is for a map to have a function like
TryInsert that returns a bool if the insert succeeded.  Does the user care
whether the insert succeeded?  Sometimes.

You may be right about "the vast majority of functions that return a
StringRef".  (I'm not saying you are, btw, just that I don't have any data
for or against, so maybe you are, and maybe not).  But certainly not all
functions, for the same reason that you might not check any other return
value.  What makes StringRef different than bool, int, Foo, or std::string
in this regard?

llvm::Error is a good example of a class where LLVM_NODISCARD makes sense
at the class.  StringRef, however, I strongly disagree with.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/0b1e4341/attachment.html>