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

[Mehdi Amini via llvm-commits]

> On Nov 28, 2016, at 8:59 AM, Zachary Turner <zturner at google.com> wrote:
> 
> 
> 
> On Sun, Nov 27, 2016 at 10:53 PM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> On Nov 27, 2016, at 10:24 PM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
>> 
>> 
>> On Sun, Nov 27, 2016 at 8:36 PM Mehdi Amini <mehdi.amini at apple.com <mailto: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 <mailto: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. 

That’s not much different from what I said I think, basically look at the existing use to decide if it makes sense or not.

> So I don't think this is a useful metric either for or against.

I don’t get that.


> 
>  
>> 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.

We’re not designing anything from scratch here, we have a large codebase, I believe this is enough to drive these kind of decision.

— 
Mehdi


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/75552f3b/attachment.html>