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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

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.

Probably not (yet) practical, but I wonder if `ConstString::ConstString(const char *)` could be `constexpr` in the future or if the side effect of manipulating the string pool would always prohibit that.

I'd love to see the static const variables for the short constant strings, just to weigh readability hit.



================
Comment at: lldb/include/lldb/Utility/ConstString.h:173
+    // StringRef doesn't. Therefore we have to do this check manually now.
+    if (!m_string ^ !rhs.data())
+      return false;
----------------
This is very clever way to express the constraint, but it's quite a bit of cognitive load for the reader to figure out what's being tested here.  The comment sort of explains, but leaves it up to the reader to see how that maps to the condition.  (It also leaves me wondering whether it's useful to have ConstString treat empty strings and nullptr as distinct things.)

A simpler way to communicate the condition to humans might be:

    if (m_string == nullptr && rhs.data() != nullptr) return false;
    if (m_string != nullptr && rhs.data() == nullptr) return false;

The clever expression requires the reader to know or deduce:

1.  that `m_string` and `rhs.data()` are pointers,
2.  that `!p` is equivalent to `p != 0` and that the `0` will be implicitly converted to `nullptr`, yielding a `bool` that's `true` if `p` is a nullptr,
3.  that `^` is bitwise exclusive or,
4.  that boolean `!` and bitwise `^` have the appropriate operator precedence, which is not always obvious when mixing types like this,
5.  that `true` and `false` are guaranteed to have values such that bitwise `^` will do the expected thing to the promoted types,
6.  and that the resulting `int` will be implicitly compared for inequality to 0.

Most of these are reasonable things to expect a C++ programmer to know, but expecting them to apply all of that knowledge (correctly) to figure out what the expression does seems like an unnecessarily high cognitive burden.


================
Comment at: lldb/source/Core/Disassembler.cpp:1407
     return (op.m_type == Instruction::Operand::Type::Register &&
-            (op.m_register == ConstString(info.name) ||
-             op.m_register == ConstString(info.alt_name)));
+            (op.m_register == info.name || op.m_register == info.alt_name));
   };
----------------
This is _probably_ a performance win here, but it's not obvious.  If the resulting lambda is called many times, and it actually captured `ConstString(info.name)` and `ConstString(info.alt_name)` rather than `info.name` and `info.alt_name`, then we're trying off two pointer comparisons for two StringRef comparisons.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60667