labath added a reviewer: jingham.
labath added a subscriber: jingham.
labath added a comment.
+ @jingham for value object stuff
Though I agree with the general principle of this change, the new api which
implements it seems extremely confusing. Unfortunately, it's not really clear
to me how to improve that. Maybe it would be better if the `Get` methods
returned Expected<lldb::TypeFormatImplSP>? Then an "error" would mean ambiguous
match, valid pointer would be "ok", and a null pointer would be "no match"? Or
maybe both "no match" and "ambiguous match" should be errors? Or maybe, since
these objects claim to be containers, they shouldn't even treat multiple
matches as an error, but instead provide an interface to get *all* matches, and
then this can be converted to an error higher up?
================
Comment at: lldb/include/lldb/Core/ValueObject.h:460
+ // Store error from Expected<> parameter unless some error is already stored.
+ template <class T> void TakeError(llvm::Expected<T> &&e) {
+ if (!e && !GetError().Fail())
----------------
I didn't mention this in the previous patches, but I think you're overusing the
rvalue references here. It's generally better to use regular value objects in
cases like these because this enforces proper contracts between functions.
Using rvalue references leaves open the possibility for a bug (which you've
actually made here) that the `TakeError` function will do nothing to the
passed-in object (on some path of execution), which means that the object will
continue to exist in the "untaken" state in the caller, and then probably
assert some distance away from where it was meant to be used.
Passing the argument as a plain object means that this function *must* do
something with it (really *take* it) or *it* will assert. The cost of that is a
single object move, but that should generally be trivial for objects that
implement moving properly.
================
Comment at: lldb/include/lldb/DataFormatters/FormatClasses.h:176
+ lldb::RegularExpressionSP m_regex1, m_regex2;
+ ConstString m_str;
+};
----------------
std::string, please. :) ConstString is severely overused in lldb..
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66654/new/
https://reviews.llvm.org/D66654
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
