[llvm] r284364 - ADT: Use LLVM_NODISCARD instead of LLVM_ATTRIBUTE_UNUSED_RESULT for StringRef
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 27 20:36:54 PST 2016
> On Nov 27, 2016, at 6:24 PM, Justin Bogner via llvm-commits <llvm-commits at lists.llvm.org> 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?
Many containers have an insert() function that returns two values: an iterator and a boolean to indicate that it was inserted.
The returned value is a convenience that “may” be useful to the caller, but it seems quite legitimate to insert() without needed either the iterator or the bool.
Are you suggesting that a good design would be the following?
StringRef insertAndReturnHash(...);
void insert(...) {
(void)insertAndReturnHash(...);
}
The same insert() pattern could be found in a StringSet that would take a StringRef, make a copy, insert, and return a StringRef to the owned copy. Again ignoring the returned value seems legit to me.
> 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!
—
Mehdi
>
>> LMK your thoughts on this, the change was done in r287856
>
> I think you mean r287586.
>
>> On Sun, Oct 16, 2016 at 11:44 PM Justin Bogner via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: bogner
>>> Date: Mon Oct 17 01:35:23 2016
>>> New Revision: 284364
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=284364&view=rev
>>> Log:
>>> ADT: Use LLVM_NODISCARD instead of LLVM_ATTRIBUTE_UNUSED_RESULT for
>>> StringRef
>>>
>>> Instead of annotating (most of) the StringRef API, we can just
>>> annotate the type directly. This is less code and it will warn in more
>>> cases.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/ADT/StringRef.h
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/StringRef.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringRef.h?rev=284364&r1=284363&r2=284364&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/StringRef.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/StringRef.h Mon Oct 17 01:35:23 2016
>>> @@ -44,7 +44,7 @@ namespace llvm {
>>> /// situations where the character data resides in some other buffer,
>>> whose
>>> /// lifetime extends past that of the StringRef. For this reason, it is
>>> not in
>>> /// general safe to store a StringRef.
>>> - class StringRef {
>>> + class LLVM_NODISCARD StringRef {
>>> public:
>>> typedef const char *iterator;
>>> typedef const char *const_iterator;
>>> @@ -281,8 +281,8 @@ namespace llvm {
>>> ///
>>> /// \returns The index of the first character satisfying \p F
>>> starting from
>>> /// \p From, or npos if not found.
>>> + LLVM_NODISCARD
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> size_t find_if(function_ref<bool(char)> F, size_t From = 0) const {
>>> StringRef S = drop_front(From);
>>> while (!S.empty()) {
>>> @@ -297,8 +297,8 @@ namespace llvm {
>>> ///
>>> /// \returns The index of the first character not satisfying \p F
>>> starting
>>> /// from \p From, or npos if not found.
>>> + LLVM_NODISCARD
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> size_t find_if_not(function_ref<bool(char)> F, size_t From = 0) const
>>> {
>>> return find_if([F](char c) { return !F(c); }, From);
>>> }
>>> @@ -500,7 +500,6 @@ namespace llvm {
>>> /// exceeds the number of characters remaining in the string, the
>>> string
>>> /// suffix (starting with \p Start) will be returned.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef substr(size_t Start, size_t N = npos) const {
>>> Start = std::min(Start, Length);
>>> return StringRef(Data + Start, std::min(N, Length - Start));
>>> @@ -510,7 +509,6 @@ namespace llvm {
>>> /// elements remaining. If \p N is greater than the length of the
>>> /// string, the entire string is returned.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef take_front(size_t N = 1) const {
>>> if (N >= size())
>>> return *this;
>>> @@ -521,7 +519,6 @@ namespace llvm {
>>> /// elements remaining. If \p N is greater than the length of the
>>> /// string, the entire string is returned.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef take_back(size_t N = 1) const {
>>> if (N >= size())
>>> return *this;
>>> @@ -531,7 +528,6 @@ namespace llvm {
>>> /// Return the longest prefix of 'this' such that every character
>>> /// in the prefix satisfies the given predicate.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef take_while(function_ref<bool(char)> F) const {
>>> return substr(0, find_if_not(F));
>>> }
>>> @@ -539,7 +535,6 @@ namespace llvm {
>>> /// Return the longest prefix of 'this' such that no character in
>>> /// the prefix satisfies the given predicate.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef take_until(function_ref<bool(char)> F) const {
>>> return substr(0, find_if(F));
>>> }
>>> @@ -547,7 +542,6 @@ namespace llvm {
>>> /// Return a StringRef equal to 'this' but with the first \p N
>>> elements
>>> /// dropped.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef drop_front(size_t N = 1) const {
>>> assert(size() >= N && "Dropping more elements than exist");
>>> return substr(N);
>>> @@ -556,7 +550,6 @@ namespace llvm {
>>> /// Return a StringRef equal to 'this' but with the last \p N elements
>>> /// dropped.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef drop_back(size_t N = 1) const {
>>> assert(size() >= N && "Dropping more elements than exist");
>>> return substr(0, size()-N);
>>> @@ -565,7 +558,6 @@ namespace llvm {
>>> /// Return a StringRef equal to 'this', but with all characters
>>> satisfying
>>> /// the given predicate dropped from the beginning of the string.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef drop_while(function_ref<bool(char)> F) const {
>>> return substr(find_if_not(F));
>>> }
>>> @@ -573,15 +565,14 @@ namespace llvm {
>>> /// Return a StringRef equal to 'this', but with all characters not
>>> /// satisfying the given predicate dropped from the beginning of the
>>> string.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef drop_until(function_ref<bool(char)> F) const {
>>> return substr(find_if(F));
>>> }
>>>
>>> /// Returns true if this StringRef has the given prefix and removes
>>> that
>>> /// prefix.
>>> + LLVM_NODISCARD
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> bool consume_front(StringRef Prefix) {
>>> if (!startswith(Prefix))
>>> return false;
>>> @@ -592,8 +583,8 @@ namespace llvm {
>>>
>>> /// Returns true if this StringRef has the given suffix and removes
>>> that
>>> /// suffix.
>>> + LLVM_NODISCARD
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> bool consume_back(StringRef Suffix) {
>>> if (!endswith(Suffix))
>>> return false;
>>> @@ -614,7 +605,6 @@ namespace llvm {
>>> /// will be returned. If this is less than \p Start, an empty string
>>> will
>>> /// be returned.
>>> LLVM_ATTRIBUTE_ALWAYS_INLINE
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef slice(size_t Start, size_t End) const {
>>> Start = std::min(Start, Length);
>>> End = std::min(std::max(Start, End), Length);
>>> @@ -709,42 +699,36 @@ namespace llvm {
>>>
>>> /// Return string with consecutive \p Char characters starting from
>>> the
>>> /// the left removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef ltrim(char Char) const {
>>> return drop_front(std::min(Length, find_first_not_of(Char)));
>>> }
>>>
>>> /// Return string with consecutive characters in \p Chars starting
>>> from
>>> /// the left removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef ltrim(StringRef Chars = " \t\n\v\f\r") const {
>>> return drop_front(std::min(Length, find_first_not_of(Chars)));
>>> }
>>>
>>> /// Return string with consecutive \p Char characters starting from
>>> the
>>> /// right removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef rtrim(char Char) const {
>>> return drop_back(Length - std::min(Length, find_last_not_of(Char) +
>>> 1));
>>> }
>>>
>>> /// Return string with consecutive characters in \p Chars starting
>>> from
>>> /// the right removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef rtrim(StringRef Chars = " \t\n\v\f\r") const {
>>> return drop_back(Length - std::min(Length, find_last_not_of(Chars)
>>> + 1));
>>> }
>>>
>>> /// Return string with consecutive \p Char characters starting from
>>> the
>>> /// left and right removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef trim(char Char) const {
>>> return ltrim(Char).rtrim(Char);
>>> }
>>>
>>> /// Return string with consecutive characters in \p Chars starting
>>> from
>>> /// the left and right removed.
>>> - LLVM_ATTRIBUTE_UNUSED_RESULT
>>> StringRef trim(StringRef Chars = " \t\n\v\f\r") const {
>>> return ltrim(Chars).rtrim(Chars);
>>> }
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list