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

Zachary Turner via llvm-commits llvm-commits at lists.llvm.org
Mon May 15 12:42:08 PDT 2017


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


More information about the llvm-commits mailing list