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

Justin Bogner via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 16 15:41:11 PDT 2017


I'm a little late to the discussion, but I pretty strongly agree with
Rui's suggestion that returning an Optional is a better interface here.
I'm not really convinced that the cases where we can actually deduce a
type come up often enough that there's a significant useability
improvement.

Zachary Turner via llvm-commits <llvm-commits at lists.llvm.org> writes:
> 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;
> }

How is this any less error prone than the following?

  int DoSomething(StringRef Str) {
    unsigned i;
    if (to_integer(Str, i))
      return i;
    // handle error
    return -1;
  }

I'd even argue that this is a little worse, since the type name and the
assignment to the variable aren't in the same statement anymore.

> 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.

Sure, this is the case where deduction actually works, but how often
does this really come up? You generally want to do something with the
value rather than just stuff it in a member, except perhaps in cases
that are pretty straightforward parsers. My assumption here seems to be
backed up from a very quick and unscientific grep of llvm for
getAsInteger, which almost always declares the uninitialized variable
immediately before the call.

So if this isn't the common case, I don't think it's all that bad to
encourage people to let the compiler help them out like so:

  void Foo::bar(StringRef S) {
    if (auto Result = to_integer<decltype(Member)>(Str))
      Member = *Result;
  }

What do you think?

> 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
>>>>>
>>>>>
>>>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


More information about the llvm-commits mailing list