[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

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