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