[llvm] r303011 - [StringExtras] Add llvm::to_integer.
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Mon May 15 16:02:11 PDT 2017
By the way, shouldn't it be `toInteger`?
On Mon, May 15, 2017 at 12:53 PM, Rui Ueyama <ruiu at google.com> wrote:
> 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/01fff52a/attachment.html>
More information about the llvm-commits
mailing list