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