<div dir="ltr">It's error prone because you might have something like this:<div><br></div><div>int DoSomething(StringRef Str) {</div><div> if (auto Result = to_integer<unsigned>(Str))</div><div> return *Result;</div><div> // handle error</div><div> return -1;</div><div>}</div><div><br></div><div>As another example, you might want to store the value in a member variable.</div><div><br></div><div>void Foo::bar(StringRef S) {</div><div> if (auto Result = to_integer<unsigned>(Str))</div><div> this->Member = *Result;</div><div>}</div><div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, May 15, 2017 at 12:38 PM Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@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"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 15, 2017 at 10:56 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</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">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:<div><br></div><div>int x;</div><div>if (to_integer("37", x))</div><div><br></div><div>you would now have to write:</div><div><br></div><div>if (auto X = to_integer<int>("37"))</div><div><br></div><div>forcing the user to specify the type, which in my experience is more error prone.</div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Why is it error prone? An alternative is</div><div><br></div><div> int X;</div><div> if (to_integer("37", X))</div><div><br></div><div>but it explicitly says that X is of int, so it seems the same as your example.</div><div><br></div><div>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.</div><div><br></div><div> int X;</div><div> if (to_integer("37", X)) {</div><div> }</div><div> // X is still live here</div><div><br></div><div>seems better than</div><div><br></div><div> if (auto X = to_integer<int>("37")) {</div><div> }</div><div> // X is no longer defined.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Perhaps there could be an overload though? It seems like we could support both syntaxes.</div><div><br></div><div>template<typename T></div><div>bool to_integer(StringRef S, T &Result, unsigned Radix = 0) {</div><div> return !S.getAsInteger(Radix, Result);</div><div>}</div><div><br></div><div>template<typename T></div><div>Optional<T> to_integer(StringRef S, unsigned Radix = 0) {</div><div> T Result;</div><div> return to_integer(S, Result, Radix) ? Result : None;</div><div>}</div><div><br></div><div>both one problem with this might be that this will be ambiguous if someone writes:</div><div><br></div><div>unsigned N;</div><div>auto X = to_integer("37", N);</div><div><br></div><div>Not sure if there's a convenient way to resolve this ambiguity.</div></div><div class="m_8144627858609984622HOEnZb"><div class="m_8144627858609984622h5"><br><div class="gmail_quote"><div dir="ltr">On Mon, May 15, 2017 at 10:36 AM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@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">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="m_8144627858609984622m_-1734610346841291005m_-8970066819742217949HOEnZb"><div class="m_8144627858609984622m_-1734610346841291005m_-8970066819742217949h5"><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-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(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/StringExtras.h<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/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-project/llvm/trunk/include/llvm/ADT/StringExtras.h?rev=303011&r1=303010&r2=303011&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ADT/StringExtras.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/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>
_______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></blockquote></div></blockquote></div>
</div></div><br>_______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div></div></div></blockquote></div>