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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
