[llvm] r303011 - [StringExtras] Add llvm::to_integer.
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Mon May 15 12:37:52 PDT 2017
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/6b7ecb30/attachment.html>
More information about the llvm-commits
mailing list