[llvm] r303011 - [StringExtras] Add llvm::to_integer.
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Tue May 16 09:13:08 PDT 2017
I have a very mild preference to follow the pattern of std::to_string, but
happy to defer to others if they have strong opinions.
On Mon, May 15, 2017 at 8:52 PM Zachary Turner <zturner at google.com> wrote:
> 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/4ad09b91/attachment.html>
More information about the llvm-commits
mailing list