[Lldb-commits] [PATCH] D60667: Allow direct comparison of ConstString against StringRef

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

In D60667#1467970 <https://reviews.llvm.org/D60667#1467970>, @teemperor wrote:

> In D60667#1467387 <https://reviews.llvm.org/D60667#1467387>, @amccarth wrote:
>
> > I, too, have some concern that this could have unintended side effects.  To make the temporary `StringRef`s from the zero-terminated strings requires a `strlen` call each time.  So we're making two passes over a string each time (once to measure and once to compare).  Granted, these are mostly very short.
>
>
> I think I'm not understanding something here. There is one `strlen` call to make the StringRef from the literal, but I don't see why we need a second one to compare (as StringRef and ConstString both store the string length and don't recompute it when comparing). Simple godbolt test case here: https://godbolt.org/z/9X1RRt So both the old and new version both require one `strlen` call from what I can see (which is anyway optimized out for comparing against literals).


I didn't mean to imply that `strlen` is called twice.  My point was that there are two linear passes over the string literal:  The first is the call to `strlen` to make the StringRef, and the second is the actual string comparison.

> 
> 
>> I'd love to see the static const variables for the short constant strings, just to weigh readability hit.
> 
> I can maybe make a new revision to show this, but I hope it's obvious from looking at the code that I changed in this revision. Also there are the problems/style difficulties that hopefully don't need a full revision to see:
> 
> 1. We preferable want to have variables with the same name as the string content, but often the strings contain reserved words or words that don't translate into our lower_case_code_style so that's not possible. E.g. `var == this_` is not really as readable as `var == "this"` (and it looks like a typo). Same with stuff like `if (arg_type.GetTypeName() == "bool")` vs `if (arg_type.GetTypeName() == bool_)` or `image_infos[i].segments[k].name == ConstString("__TEXT"))` vs `image_infos[i].segments[k].name == text)`.
> 2. We get a bunch of local variables that just bloat the code: ``` if (descriptor->IsCFType()) { ConstString type_name(valobj.GetTypeName()); if (type_name == "__CFMutableBitVector" || type_name == "__CFBitVector" || type_name == "CFMutableBitVectorRef" || type_name == "CFBitVectorRef") {
> 
> 
> 
>       if (valobj.IsPointerType())
>         is_type_ok = true;
>     }
>   }
> 
>   becomes this
> 
> 
>   if (descriptor->IsCFType()) {
>     ConstString type_name(valobj.GetTypeName());
>     static const ConstString cf_mutable_bit_vector("__CFMutableBitVector"),
>                              cf_bit_vector("__CFBitVector"),
>                              cf_mutable_bit_vector_ref("CFMutableBitVectorRef"),
>                              cf_bit_vector_ref("CFBitVectorRef")
>     if (type_name == cf_mutable_bit_vector || type_name == cf_bit_vector ||
>         type_name == cf_mutable_bit_vector_ref || type_name == cf_bit_vector_ref) {
>       if (valobj.IsPointerType())
>         is_type_ok = true;
>     }
>   }
> 
>   3. Code becomes much harder to grep. E.g. if I want to check for the location where we filter the `this` variable from the local variables, I can currently do `git grep "\"this\"" | grep ==` and directly see where we do the comparison. But with local variables I have to do some funky grep with context filtering. In general grepping for these string literals without a few lines of context will be pointless with ConstString variables for literals.

Fair enough.  Thanks for the details.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60667/new/

https://reviews.llvm.org/D60667