<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 17, 2016 at 12:38 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Oct 17, 2016, at 10:12 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_2465380976656664214Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">(for my money the readability loss of adding StringRef(...) all over the place doesn't seem quite worth it, but I'm not going to stand in the way of the change or anything.)<br class="gmail_msg"></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">The balance is between the readability, and the “error prone” current behavior.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">For example we introduced all these redundant .c_str() in the past: <a href="https://reviews.llvm.org/D25667" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25667</a></div><div class="gmail_msg">Ideally we shouldn’t have much explicit calls to StringRef, if all the APIs are converted to work with StringRef most of the time. </div></div></div></blockquote><div><br></div><div>But for now we do have a fair few (judging by the size of this change).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><br class="gmail_msg">Are these changes necessarily joined - or is the explicitness of the StringRef(...) ctor independent of the addition of the template?<br class="gmail_msg"></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">It is not independent: the template is never selected if the other constructor is not made explicit.</div></div></div></blockquote><div><br>Ah, because the const char* is a close enough match that the template isn't chosen.<br><br>Can workaround that with something like:<br><br> template<typename T></div><div>  StringRef(T *Str, typename std::enable_if<std::is_same<T, const char>::value>::type = 0) = delete;<br><br>(now it's a template, so it's not a better match than the other one - but wouldn't trigger other implicit conversions - could generalize it to T Str, is_convertible_to<T, const char*>, etc)</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">Do we need to worry about the recursive strlen in the template ctor? Or is it clear that even in a non-constexpr context the compilers we care about produce reasonable performance?</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">Can you clarify what you mean by "non-constexpr context” here?</div><div class="gmail_msg">The recursive template is not intended to be used with anything else than literal, I am not expecting this to generate any code ever (if it does, it is not intended).</div></div></div></blockquote><div><br>Constexpr functions don't guarantee that they will be evaluated at compile time. They only provide that they /can/ be used in a place where compile time evaluation is required (& taht they will conform to the constexpr requirements (not writing mutating state, throwing exceptions, etc) in whatever places they're used that require constexpr (eg: a constexpr function can throw under some condition, so long as all the uses of the function in a constexpr context don't satisfy that condition))<br><br>For example, look at the IR generated from this code:<br><br><div>struct StringRef {</div><div>  constexpr static unsigned getInitLength(char const *str, unsigned count = 0) {</div><div>    return ('\0' == str[0]) ? count : getInitLength(str + 1, count + 1);</div><div>  }</div><div><br></div><div>  template <int N></div><div>  constexpr StringRef(const char (&Str)[N]) </div><div>    : Length(getInitLength(Str)) {</div><div>  }</div><div><br></div><div>  int Length;</div><div>};</div><div><br></div><div>int main() {</div><div>  StringRef S = "foo";</div><div>}<br><br><div>$ grep getInitLength str.ll</div><div>$_ZN9StringRef13getInitLengthEPKcj = comdat any</div><div>  %call = call i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %arraydecay, i32 0)</div><div>define linkonce_odr i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %str, i32 %count) #1 comdat align 2 {</div><div>  %call = call i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %add.ptr, i32 %add)<br><br>the getInitLength function exists and is recursive (the only way to force this not to happen is if the S variable itself is declared constexpr, then there's no mention of getInitLength in the generated IR).<br><br>Granted, if it's always a compile time constant string and everything is inlinable, etc, the optimizer will get rid of it - but LLVM's -O0 build might suffer greatly with all this recursive string length evaluation.<br><br>& of course any code like this wouldn't ever be optimized away & would still hit the recursive version (which might be optimized to a loop - /maybe/ even to strlen?):<br><br>char x[N];<br>x[0] = 'a' + (rand() % 26); // or otherwise compute string contents at runtime<br>StringRef S = x;<br><br>(essentially what Zach was asking about/pointing out)<br><br>Does all of that make sense? I may've done a poor job explaining it.</div></div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">— </div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">Mehdi</div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Mon, Oct 17, 2016 at 12:57 AM Manuel Klimek via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">klimek added a comment.<br class="gmail_msg">
<br class="gmail_msg">
In <a href="https://reviews.llvm.org/D25639#571344" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25639#571344</a>, @zturner wrote:<br class="gmail_msg">
<br class="gmail_msg">
> The only thing I'm not crazy about here is that the clang tidy check seems to blindly replace all calls to `s.c_str()` with `llvm::StringRef(s.c_str())`.  Is there any way to make it attempt to replace it with just `s` first, and only if that fails do you then try `llvm::StringRef(s.c_str())`?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
Shouldn't it be relatively straight forward to discover whether the expression is convertible to StringRef without the .c_str() call from an AST matcher?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D25639" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25639</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
_______________________________________________<br class="gmail_msg">
llvm-commits mailing list<br class="gmail_msg">
<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="gmail_msg">
</blockquote></div>
</div></blockquote></div></div></blockquote></div></div>