[llvm] r303011 - [StringExtras] Add llvm::to_integer.

[Rui Ueyama via llvm-commits]

It is indeed better than StringRef::getAsInteger whose I found its "return
true on failure" semantics is confusing.

But I really hope the new function returns not `bool` but `Optional<N>` so
that it returns a result as a return value instead of destructively
mutating a given argument via a reference. Can we change that?

On Mon, May 15, 2017 at 9:59 AM, Zachary Turner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> It may in fact be better to remove StringRef.getAsInteger.  But that is
> quite a large change, and it will involve inverting the conditional at
> every single call site, which will be high risk.
>
> On Mon, May 15, 2017 at 9:59 AM Zachary Turner <zturner at google.com> wrote:
>
>> My commit message may have been unclear.  "This function" refers to the
>> NEW function.  The new function does in fact return true on success.  It's
>> StringRef::getAsInteger that doesn't, that's the mistake I was referring to.
>>
>> On Mon, May 15, 2017 at 9:57 AM David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> On Sun, May 14, 2017 at 10:24 AM Zachary Turner via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: zturner
>>>> Date: Sun May 14 12:11:05 2017
>>>> New Revision: 303011
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=303011&view=rev
>>>> Log:
>>>> [StringExtras] Add llvm::to_integer.
>>>>
>>>> This is a very thin wrapper around StringRef::getAsInteger.
>>>> It serves three purposes.
>>>>
>>>> 1) It allows a cleaner syntax when you have something other than
>>>>    a StringRef - for example, a std::string or an llvm::SmallString.
>>>>    Previously, in this case you would have to write something like:
>>>>       StringRef(MyStr).getAsInteger(0, Result)
>>>>    by explicitly constructing a temporary StringRef.  This can be
>>>>    done implicitly however with the new function by just writing:
>>>>       to_integer(MyStr, ...).
>>>>
>>>
>>> For this ^ it seems like it'd be better, maybe, to remove the member
>>> version and just have the non-member version?
>>>
>>>
>>>> 2) Correcting the travesty that is getAsInteger's return value.
>>>>    This function returns true on success, and false on failure.
>>>>    While this may cause confusion with people familiar with the
>>>>    getAsInteger API, there seems to be widespread agreement that
>>>>    the return semantics of getAsInteger was a mistake.
>>>>
>>>
>>> ^ this might be less obvious. LLVM had/has a long history of that
>>> true-on-success return bool in some parts of the codebase... :/
>>>
>>> Though I guess there's probably enough inconsistency there these days
>>> that maybe it's fine to lean more heavily towards non-zero (true) on
>>> failure.
>>>
>>>
>>>> 3) It allows the Radix to be deduced as a default argument by
>>>>    putting it last in the parameter list.  Most uses of getAsInteger
>>>>    pass 0 for the first argument.  With this syntax it can just be
>>>>    omitted.
>>>>
>>>> Modified:
>>>>     llvm/trunk/include/llvm/ADT/StringExtras.h
>>>>
>>>> Modified: llvm/trunk/include/llvm/ADT/StringExtras.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
>>>> llvm/ADT/StringExtras.h?rev=303011&r1=303010&r2=303011&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- llvm/trunk/include/llvm/ADT/StringExtras.h (original)
>>>> +++ llvm/trunk/include/llvm/ADT/StringExtras.h Sun May 14 12:11:05 2017
>>>> @@ -106,6 +106,13 @@ static inline std::string fromHex(String
>>>>    return Output;
>>>>  }
>>>>
>>>> +/// \brief Convert the string \p S to an integer of the specified type
>>>> using
>>>> +/// the radix \p Base.  If \p Base is 0, auto-detects the radix.
>>>> +/// Returns true if the number was successfully converted, false
>>>> otherwise.
>>>> +template <typename N> bool to_integer(StringRef S, N &Num, unsigned
>>>> Base = 0) {
>>>> +  return !S.getAsInteger(Base, Num);
>>>> +}
>>>> +
>>>>  static inline std::string utostr(uint64_t X, bool isNeg = false) {
>>>>    char Buffer[21];
>>>>    char *BufPtr = std::end(Buffer);
>>>>
>>>>
>>>> _______________________________________________
>>>> 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/20170515/5416949a/attachment.html>