<div dir="ltr">It is indeed better than StringRef::getAsInteger whose I found its "return true on failure" semantics is confusing.<div><br></div><div>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?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 15, 2017 at 9:59 AM, Zachary Turner via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Mon, May 15, 2017 at 9:59 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.</div><br><div class="gmail_quote"><div dir="ltr">On Mon, May 15, 2017 at 9:57 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, May 14, 2017 at 10:24 AM Zachary Turner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: zturner<br>
Date: Sun May 14 12:11:05 2017<br>
New Revision: 303011<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=303011&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=303011&view=rev</a><br>
Log:<br>
[StringExtras] Add llvm::to_integer.<br>
<br>
This is a very thin wrapper around StringRef::getAsInteger.<br>
It serves three purposes.<br>
<br>
1) It allows a cleaner syntax when you have something other than<br>
a StringRef - for example, a std::string or an llvm::SmallString.<br>
Previously, in this case you would have to write something like:<br>
StringRef(MyStr).getAsInteger(<wbr>0, Result)<br>
by explicitly constructing a temporary StringRef. This can be<br>
done implicitly however with the new function by just writing:<br>
to_integer(MyStr, ...).<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>For this ^ it seems like it'd be better, maybe, to remove the member version and just have the non-member version?<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) Correcting the travesty that is getAsInteger's return value.<br>
This function returns true on success, and false on failure.<br>
While this may cause confusion with people familiar with the<br>
getAsInteger API, there seems to be widespread agreement that<br>
the return semantics of getAsInteger was a mistake.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>^ this might be less obvious. LLVM had/has a long history of that true-on-success return bool in some parts of the codebase... :/<br><br>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.<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3) It allows the Radix to be deduced as a default argument by<br>
putting it last in the parameter list. Most uses of getAsInteger<br>
pass 0 for the first argument. With this syntax it can just be<br>
omitted.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/<wbr>StringExtras.h<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/<wbr>StringExtras.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringExtras.h?rev=303011&r1=303010&r2=303011&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/ADT/StringExtras.h?rev=<wbr>303011&r1=303010&r2=303011&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/ADT/<wbr>StringExtras.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/<wbr>StringExtras.h Sun May 14 12:11:05 2017<br>
@@ -106,6 +106,13 @@ static inline std::string fromHex(String<br>
return Output;<br>
}<br>
<br>
+/// \brief Convert the string \p S to an integer of the specified type using<br>
+/// the radix \p Base. If \p Base is 0, auto-detects the radix.<br>
+/// Returns true if the number was successfully converted, false otherwise.<br>
+template <typename N> bool to_integer(StringRef S, N &Num, unsigned Base = 0) {<br>
+ return !S.getAsInteger(Base, Num);<br>
+}<br>
+<br>
static inline std::string utostr(uint64_t X, bool isNeg = false) {<br>
char Buffer[21];<br>
char *BufPtr = std::end(Buffer);<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></blockquote></div></blockquote></div>
</div></div><br>______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>