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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Mon May 15 12:53:57 PDT 2017 

Ah, makes sense. You could do `to_integer<typeof this->Member>(Str)`, but
it is ugly.

On Mon, May 15, 2017 at 12:42 PM, Zachary Turner <zturner at google.com> wrote:

> It's error prone because you might have something like this:
>
> int DoSomething(StringRef Str) {
>   if (auto Result = to_integer<unsigned>(Str))
>     return *Result;
>   // handle error
>   return -1;
> }
>
> As another example, you might want to store the value in a member variable.
>
> void Foo::bar(StringRef S) {
>   if (auto Result = to_integer<unsigned>(Str))
>     this->Member = *Result;
> }
>
> But it's possible Member is not an unsigned.  Maybe it's a size_t, or an
> int, or something else entirely, but the implicit conversion works.  When
> the type is deduced, you can't make this mistake.
>
> On Mon, May 15, 2017 at 12:38 PM Rui Ueyama <ruiu at google.com> wrote:
>
>> On Mon, May 15, 2017 at 10:56 AM, Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> 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.
>>>
>>
>> Why is it error prone? An alternative is
>>
>>   int X;
>>   if (to_integer("37", X))
>>
>> but it explicitly says that X is of int, so it seems the same as your
>> example.
>>
>> One advantage of making it return Optional<N> instead of bool is that in
>> many situations you can keep variables scope smaller by defining them in
>> `if` just like you did above. I.e.
>>
>>   int X;
>>   if (to_integer("37", X)) {
>>   }
>>   // X is still live here
>>
>> seems better than
>>
>>   if (auto X = to_integer<int>("37")) {
>>   }
>>   // X is no longer defined.
>>
>> 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/ffe7678b/attachment.html>

More information about the llvm-commits mailing list