[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

Thanks for adding me in the loop. And thanks for looking into the fix, though this bit is a bit confusing to me:

> Ensure that we explicitly indicate that we would like the move semantics
> in the assignment. With MSVC 14.35.32215, the assignment would not
> trigger a NVRO and copy constructor would be invoked

>From https://llvm.org/doxygen/classllvm_1_1Error.html, `llvm::Error` has both copy constructor and copy assignment deleted, so I don't understand how a copy constructor could be invoked? I believe I followed https://llvm.org/docs/ProgrammersManual.html#recoverable-errors in handling the error, so if not having `std::move` there is really an issue, that page should also be updated.

But it is probably like @sgraenitz said, it is just an effect of the error not being consumed when logging is disabled.

> If logging is enabled, we are moving from the same object twice, no?



> In the if body you can not std::move() from err twice.

The doc comment of the move constructor/assignment does say "original error becomes a checked Success value, regardless of its original state" and it is exactly what happens in the code, so moving from the same error twice should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669