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

[Zachary Turner via llvm-commits]

That has the drawback though of requiring the user to specify the type via
template argument.  It makes the syntax a bit uglier in my opinion.  for
example whereas before you could write:

int x;
if (to_integer("37", x))

you would now have to write:

if (auto X = to_integer<int>("37"))

forcing the user to specify the type, which in my experience is more error
prone.

Perhaps there could be an overload though?  It seems like we could support
both syntaxes.

template<typename T>
bool to_integer(StringRef S, T &Result, unsigned Radix = 0) {
  return !S.getAsInteger(Radix, Result);
}

template<typename T>
Optional<T> to_integer(StringRef S, unsigned Radix = 0) {
   T Result;
   return to_integer(S, Result, Radix) ? Result : None;
}

both one problem with this might be that this will be ambiguous if someone
writes:

unsigned N;
auto X = to_integer("37", N);

Not sure if there's a convenient way to resolve this ambiguity.

On Mon, May 15, 2017 at 10:36 AM Rui Ueyama <ruiu at google.com> wrote:

> 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/87a4f499/attachment.html>