[llvm] r284364 - ADT: Use LLVM_NODISCARD instead of LLVM_ATTRIBUTE_UNUSED_RESULT for StringRef
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 27 23:09:13 PST 2016
Mehdi Amini <mehdi.amini at apple.com> writes:
>> 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.
This is a common enough pattern that it's interesting, but I haven't
seen much of this outside of the iterator/bool pair.
It would certainly make it difficult to add nodiscard to iterator or
bool types, and I think the only way to fix it would be to add a second
attribute that marked some particular function's return value as
discardable even though the type is nodiscard. In any case though, I
think that's somewhat separate from the discussion at hand.
> 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(...);
> }
Something like that. Depending on the kind of hash table you could also
do something like this:
StringRef calculateHash(T Value); // Or maybe HashType instead of StringRef
void insert(T V, HashType H = None) {
if (H.isNone)
H = calculateHash(V);
...
}
Really though, you'd probably return an iterator/bool pair ;)
> 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