<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Oct 17, 2016, at 1:51 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, Oct 17, 2016 at 12:38 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" class="">mehdi.amini@apple.com</a>> wrote:<br class=""></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="gmail_msg m_2465380976656664214Apple-interchange-newline"><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 class=""><br class=""></div><div class="">But for now we do have a fair few (judging by the size of this change).</div></div></div></div></blockquote><div><br class=""></div><div>Sure, at least it will be easy to spot which APIs / code can benefit from an upgrade.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> </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 class=""><br class="">Ah, because the const char* is a close enough match that the template isn't chosen.<br class=""><br class="">Can workaround that with something like:<br class=""><br class=""> template<typename T></div><div class=""> StringRef(T *Str, typename std::enable_if<std::is_same<T, const char>::value>::type = 0) = delete;</div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">(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></div></div></div></blockquote><div><br class=""></div><div><div>I’m not sure what you’re looking to achieve here?</div><div class="">Assuming your template above replaces the one I made explicit, you’re forbidding creating a StringRef from a const char *?</div><div class=""><br class=""></div></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><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 class=""><br class="">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 class=""></div></div></div></div></blockquote><div><br class=""></div><div>Right, I was expecting that calling this function when instantiating using a `constexpr StringRef` constructor would be enough to trigger it, but not in all cases apparently.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class="">For example, look at the IR generated from this code:<br class=""><br class=""><div class="">struct StringRef {</div><div class=""> constexpr static unsigned getInitLength(char const *str, unsigned count = 0) {</div><div class=""> return ('\0' == str[0]) ? count : getInitLength(str + 1, count + 1);</div><div class=""> }</div><div class=""><br class=""></div><div class=""> template <int N></div><div class=""> constexpr StringRef(const char (&Str)[N]) </div><div class=""> : Length(getInitLength(Str)) {</div><div class=""> }</div><div class=""><br class=""></div><div class=""> int Length;</div><div class="">};</div><div class=""><br class=""></div><div class="">int main() {</div><div class=""> StringRef S = "foo";</div><div class="">}<br class=""><br class=""><div class="">$ grep getInitLength str.ll</div><div class="">$_ZN9StringRef13getInitLengthEPKcj = comdat any</div><div class=""> %call = call i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %arraydecay, i32 0)</div><div class="">define linkonce_odr i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %str, i32 %count) #1 comdat align 2 {</div><div class=""> %call = call i32 @_ZN9StringRef13getInitLengthEPKcj(i8* %add.ptr, i32 %add)<br class=""><br class="">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 class=""><br class="">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 class=""></div></div></div></div></div></div></blockquote><div><br class=""></div><div>Right, we can use __builtin_strlen() instead of the recursive getInitLength. I didn’t go this route because I don’t know how to address it with MSVC (and was expecting it be folded by the frontend…).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><div class=""><div class=""><br class="">& 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 class=""><br class="">char x[N];<br class="">x[0] = 'a' + (rand() % 26); // or otherwise compute string contents at runtime<br class="">StringRef S = x;<br class=""><br class="">(essentially what Zach was asking about/pointing out)<br class=""><br class="">Does all of that make sense? I may've done a poor job explaining it.</div></div></div></div></div></div></blockquote><div><br class=""></div><div>You did a good job here :)</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div></div></body></html>