[Lldb-commits] [PATCH] D66654: Prevent D66398 `TestDataFormatterStdList`-like regressions in the future

[Pavel Labath via Phabricator via lldb-commits]

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