[llvm] r303011 - [StringExtras] Add llvm::to_integer.
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Mon May 15 19:52:12 PDT 2017
I guess it probably should be according to LLVM standards. I modeled the
name after the STL function std::to_string.
Does anyone have a strong opinion either way?
On Mon, May 15, 2017 at 4:02 PM Rui Ueyama <ruiu at google.com> wrote:
> 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/20170516/b727e84f/attachment.html>
More information about the llvm-commits
mailing list