[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 10:53 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> On Nov 27, 2016, at 10:24 PM, Zachary Turner <zturner at google.com> wrote:
>
>
> 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.
>
>
> The point here is that if after years of uses in the LLVM projects, we
> never made use of an API where the no_discard does not make sense, it seems
> like good data on what make sense from an API point of view for StringRef.
> I don’t think this is irrelevant.
>
We could just as easily say that if after years of uses in the LLVM
projects, we never made use of an API where no_discard would trigger a
warning, then it is not providing a meaningful signal.  So I don't think
this is a useful metric either for or against.



> We are a library, we cannot make any assumptions about how users of our
> library use our support classes.
>
>
> Right, that’s why instead of making assumptions, looking at the codebase
> as whole answers the question perfectly.
>
Libraries, by definition, cannot possibly see all of its uses.



>
> 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 disagree with this: we’re not in the business of designing ADT for
> downstream users in the first place: if they like it and want to use it,
> great! Otherwise I believe we should put the needs of the LLVM projects
> first: we design our support libraries to make LLVM more robust, faster,
> etc. I wouldn’t want to tradeoff this for hypothetical downstream users.
>
>
> I'm not taking a position on whether or not LLVM_NODISCARD on StringRef
> leads to overall better code,
>
>
> That’s where I have a different approach, it seems to be the main trigger
> to me.
>
>
> 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?
>
>
> Consistency here is answering for each of the types you mention: “whether
> or not LLVM_NODISCARD on XYZ leads to overall better code”. Possibly
> trading-off with how many times an explicit (void) is needed at call-sites.
>

Why would it lead to better code for one and not the other?  You're going
to have to explain what's fundamentally different about these two classes
for there to be a reasonable argument that one deserves the attribute and
the other doesn't.  "Because existing code already satisfies it for X and
not for Y" is not a compelling argument.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/6b6ac0c1/attachment.html>