[Lldb-commits] [lldb] r230046 - Add an OptionValueLanguage class

[Pavel Labath]

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 "StringRef
<http://llvm.org/docs/doxygen/html/classllvm_1_1StringRef.html> - Represent
a constant reference to a string, i.e. a character array and a length,
which need not be null terminated.". 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.

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.

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.

pl

ps: sorry about spamming your thread enrico, please don't take it
personally :)



On 23 February 2015 at 16:32, Zachary Turner <zturner at google.com> wrote:

> 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.
>
> I'm not saying it's perfect, but I don't believe the reasons are strong
> enough to deviate from the style guide.
>
> On Mon, Feb 23, 2015 at 8:25 AM Pavel Labath <labath at google.com> wrote:
>
>> 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:
>>
>> 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.
>>
>> 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); }".
>> 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).
>>
>>
>> On 23 February 2015 at 16:01, Zachary Turner <zturner at google.com> wrote:
>>
>>> 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.
>>> On Mon, Feb 23, 2015 at 1:41 AM Pavel Labath <labath at google.com> wrote:
>>>
>>>>
>>>> On 20 February 2015 at 19:46, Enrico Granata <egranata at apple.com>
>>>> wrote:
>>>>
>>>>> +Error
>>>>> +OptionValueLanguage::SetValueFromString (llvm::StringRef value,
>>>>> VarSetOperationType op)
>>>>> +{
>>>>> +    Error error;
>>>>> +    switch (op)
>>>>> +    {
>>>>> +    case eVarSetOperationClear:
>>>>> +        Clear();
>>>>> +        break;
>>>>> +
>>>>> +    case eVarSetOperationReplace:
>>>>> +    case eVarSetOperationAssign:
>>>>> +        {
>>>>> +            LanguageType new_type = LanguageRuntime::
>>>>> GetLanguageTypeFromString(value.data());
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> 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.
>>>>
>>>> cheers,
>>>> pl
>>>> _______________________________________________
>>>> lldb-commits mailing list
>>>> lldb-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150223/7dcf6af8/attachment.html>