lemo added inline comments.
================ Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413 + section->GetName().GetCString(), + llvm::toString(Decompressor.takeError()).c_str()); + return 0; ---------------- labath wrote: > lemo wrote: > > labath wrote: > > > This needs to be `std::move(Error)`. If you built with asserts enabled > > > and hit this line, you would crash because you did not consume the > > > `Error` object. > > Can you please elaborate? std::move is just a cast (and the result of > > Error::takeValue() is already an rvalue - the error object has been already > > moved into a temporary Error instance) > The llvm manual > <http://llvm.org/docs/ProgrammersManual.html#recoverable-errors> says > ``` > All Error instances, whether success or failure, must be either checked or > moved from (via std::move or a return) before they are destructed. > Accidentally discarding an unchecked error will cause a program abort at the > point where the unchecked value’s destructor is run, making it easy to > identify and fix violations of this rule. > ... > Success values are considered checked once they have been tested (by invoking > the boolean conversion operator). > ... > Failure values are considered checked once a handler for the error type has > been activated. > ``` > The error object created on line 3407 (in the if-declaration) is neither > moved from nor has it's handler invoked. You only invoke it's bool operator, > which is not enough for it to be considered "checked" if it is in the > "failure" state. This means it will assert once it's destructor is executed. > By writing `llvm::toString(std::move(Error))`, you will "move" from the > object, thereby clearing it. (It also makes sense to print out the error that > you have just checked instead of some error from a previous step.) > > Try this pattern out on a toy program to see what happens: > ``` > if (Error E = make_error<StringError>("This is an error", > inconvertibleErrorCode())) > outs() << "I encountered an error but I am not handling it"; > ``` Thanks. I see, I was looking at the previous block. > By writing llvm::toString(std::move(Error)), you will "move" from the object, > thereby clearing it. It's a nice contract, although the "move" part was not the problem nor the solution in itself (I took a close look at the Error class, it doesn't matter how much you move it, someone has to eventually call handleErrors() on it. Conveniently, llvm::toString() does it) https://reviews.llvm.org/D50274 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
