<div dir="ltr">Don't get me wrong, I am not advocating that we abandon StringRef -- I like it very much. However, I think that assuming StringRef is null terminated will come back to bite us. Sooner or later somebody will try to pass a non-null terminated StringRef here and it will break. The description of StringRef says "<a href="http://llvm.org/docs/doxygen/html/classllvm_1_1StringRef.html" style="color:rgb(61,33,133);font-weight:bold;font-family:Verdana,Geneva,Arial,Helvetica,sans-serif;font-size:14.3999996185303px" target="_blank">StringRef</a><span style="color:rgb(0,0,0);font-family:Verdana,Geneva,Arial,Helvetica,sans-serif;font-size:14.3999996185303px"> - Represent a constant reference to a string, i.e. a character array and a length, which need not be null terminated.". </span>This does not mean that you cannot store null terminated strings in a StringRef, however, it does state that non-null terminated stringref are perfectly valid. And since they are valid, people will assume they can pass their object to any function which takes a StringRef. <div><br></div><div>Now, this is pretty similar to having a function which takes a parameter of type int, but the function can handle only ints from -47 to 47. 100 is a perfectly valid int, and people may try to pass 100 as the argument to this function. That is why it it is good practice your function with a comment saying what are the allowed values of the parameters, and in this case I would expect the author of the function to do that. The same goes for the functions taking null-terminated stringrefs -- it should at least be stated somewhere in the comment, or people will not know about this and they will try to pass unterminated refs. Alternatively, we should make a big note in the style guide which establishes this behavior as the default.</div><div><br></div><div>However, I believe this is not the best way to go forward. Usually one wants it's function to be self-documenting, and having an alignment between syntactically valid parameter values (those that the compiler will accept) and values, which are semantically valid (those which make sense) goes a long way towards that, so I think we should encourage that and try to make our functions operate on the full set of values whereever that is possible.</div><div><br></div><div>pl</div><div><br></div><div>ps: sorry about spamming your thread enrico, please don't take it personally :)</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 23 February 2015 at 16:32, 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">StringRef also provides a lot of useful member function that increase code's clarity, and interoperates nicely with other LLVM classes. I don't think there's anything fundamentally special about a debugger that would necessitate it deviating from the larger LLVM coding practices here. Well, except for the fact that it deals with many more strings than most apps, but for that we have ConstString. <br><br>I'm not saying it's perfect, but I don't believe the reasons are strong enough to deviate from the style guide.<div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Mon, Feb 23, 2015 at 8:25 AM Pavel Labath <<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I have followed the discussion about this on lldb-dev a week ago (which did not seem to have reached a definitive conclusion), but I must say I agree with Greg's interpretation:<div><br><div>When I see a "const char *", I immediately assume it is null terminated (unless accompanied by a size argument, or something like that). I think we agree on that.</div><div><br></div><div>However, this is not the case for llvm::StringRef - it already contains the length information, so I cease to care about null termination. In fact, the entire purpose of the StringRef function (in my opinion) is to enable easy processing of non-null terminated strings. You often need to take a substring of a large string and pass it to some function as a parameter. For example I can write "void foo(StringRef x) { x = x.trim(); bar(x); }".</div><div>Without StringRef I would either need to copy the string to ensure null termination or modify the receiver to take a pointer+length combo. Now if we demand that the StringRef contents be a null terminated string, we're back at square one - we again need to copy, only this time we end up passing the length parameter anyway. Then, I see no added value in using StringRef over const char * (ok, you have the length precomputed, which can save a couple of strlen()s, but that's not a big deal).</div></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 23 February 2015 at 16:01, 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">No we don't. A const char* is not guaranteed to be null terminated either. Its up to you to make sure you pass it a null terminated string. Same applies here.<br><div class="gmail_quote"><div><div>On Mon, Feb 23, 2015 at 1:41 AM Pavel Labath <<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On 20 February 2015 at 19:46, Enrico Granata <span dir="ltr"><<a href="mailto:egranata@apple.com" target="_blank">egranata@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="overflow:hidden">+Error<br>
+OptionValueLanguage::<u></u>SetValueFromString (llvm::StringRef value, VarSetOperationType op)<br>
+{<br>
+ Error error;<br>
+ switch (op)<br>
+ {<br>
+ case eVarSetOperationClear:<br>
+ Clear();<br>
+ break;<br>
+<br>
+ case eVarSetOperationReplace:<br>
+ case eVarSetOperationAssign:<br>
+ {<br>
+ LanguageType new_type = LanguageRuntime::<u></u>GetLanguageTypeFromString(<u></u>value.data());</div></blockquote></div><br></div></div><div dir="ltr"><div class="gmail_extra">Hi,</div><div class="gmail_extra"><br></div><div class="gmail_extra">llvm::StringRef::data() is not guaranteed to return a null terminated string. We would need to call value.str().c_str() here, or (even better) fix the receiver to operate on llvm::StringRef as well.</div><div class="gmail_extra"><br></div><div class="gmail_extra">cheers,</div><div class="gmail_extra">pl</div></div></div></div><span>
______________________________<u></u><u></u>_________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailm<u></u>an/listinfo/lldb-commits</a><br>
</span></blockquote></div>
</blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div><br></div>