labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land.
================ Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413 + section->GetName().GetCString(), + llvm::toString(Decompressor.takeError()).c_str()); + return 0; ---------------- lemo wrote: > 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) Cool, thanks. I'm sorry if my previous comments were a bit unclear. https://reviews.llvm.org/D50274 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
