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

Tue Aug 27 05:10:58 PDT 2019

jankratochvil planned changes to this revision.
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.

================
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())
----------------
labath wrote:
> 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.
Thanks for this notification, I see I was automatically using the && parameter specifier excessively. rvalues still work properly with std::move() even without using this parameter specifier.
I also agree there was the bug with possibly untaken Error asserting later (I find that as an orthogonal bug to the && one).

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66654