[llvm] r284364 - ADT: Use LLVM_NODISCARD instead of LLVM_ATTRIBUTE_UNUSED_RESULT for StringRef
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 28 10:13:11 PST 2016
Well, I mean for starters it assumes that our existing codebase is an
absolutely inarguable model of what's good :-/ That doesn't mean it's bad,
I certainly think we have an above average codebase, but I don't see how we
can use the mere existence of something as evidence of its quality.
On Mon, Nov 28, 2016 at 9:59 AM Mehdi Amini <mehdi.amini at apple.com> wrote:
> On Nov 28, 2016, at 9:55 AM, Zachary Turner <zturner at google.com> wrote:
>
> The real use case in question was a function very similar to StringRef's
> consumeInteger function, but instead of modifying the StringRef in place
> (e.g. by accepting a StringRef&) to point to the first portion after the
> consumed string, it returned a StringRef representing the unconsumed
> portion of the input.
>
> A caller not interested in the unconsumed portion of the input would
> simply ignore the return value.
>
> This aside, I still think this is irrelevant. If we're going to make the
> blanket decision that ignoring return values is bad, then add
> -Wunused-result-strict and try to get it turned on across the board in
> LLVM. "It's good API design to not ignore StringRef return values because
> nobody has ever ignored a StringRef return value before" is a dubious
> argument at best.
>
>
> I strongly disagree with this assessment, it has nothing dubious in my
> opinion.
>
> On the opposite, I found dubious at best that you find this dubious :p
>
> —
> Mehdi
>
>
>
> On Mon, Nov 28, 2016 at 9:52 AM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> On Nov 28, 2016, at 9:18 AM, David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> I'm going to +1 Zach here, this doesn't seem like a good use of the
> attribute.
>
> It's perfectly reasonable to have an API that returns an optionally useful
> StringRef (& I'd be fairly sure we have a few in the LLVM project already,
> but even if we don't I'd still be in favor of continuing to allow such API
> design without workarounds like void casts or extra attributes to opt out -
> especially considering templates in APIs that might expose such things
> indirectly (& possibly be unannotatable because they're in the STL))
>
>
> Since the attribute was committed for some time and no failure was
> reported, that seems quite hypothetical.
> I rather see a real use-case in our codebase before ditching something
> like that speculatively.
>
> —
> Mehdi
>
>
>
> LLVM_ATTRIBUTE_UNUSED_RESULT on a type was really designed/intended for
> types like llvm::Error (eg:
> http://google.github.io/google-api-cpp-client/latest/doxygen/classgoogleapis_1_1util_1_1Status.html ).
> It doesn't seem appropriate to put this on generic value types (& as Zach
> said - if that were the case we'd probably end up putting it on basically
> every type, and at that point should consider a generic warning for ignored
> return values - but I don't think that would be practical)
>
> - Dave
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/e2b16d1d/attachment-0001.html>
More information about the llvm-commits
mailing list