Page MenuHomePhabricator

Return Error instead of bool from mergeTypeStreams().

Authored by ruiu on Jan 31 2017, 6:15 PM.



Previously, mergeTypeStreams returns only true or false, so it was
impossible to know the reason if it failed. This patch changes the
function signature so that it returns an Error object.

Event Timeline

ruiu created this revision.Jan 31 2017, 6:15 PM
zturner added inline comments.Feb 1 2017, 10:35 AM

Should probably #include "llvm/Support/Error.h" here.

99–100 ↗(On Diff #86715)

If there is more than one error, this will trigger an exception because you will have not checked the previous value of the error. Instead of storing an Optional<Error> maybe try storing just an Error, and then saying

LastError = llvm::join_errors(std::move(LastError), llvm::make_error<CodeViewError>(cv_error_code::corrupt_record));
183 ↗(On Diff #86715)

If you do the above, you can simply return LastError here.

ruiu updated this revision to Diff 86684.Feb 1 2017, 11:23 AM
  • Updated as per Zach's review comments.
zturner added inline comments.Feb 1 2017, 11:31 AM
60 ↗(On Diff #86715)

Shouldn't we do this in the destructor instead?

ruiu added inline comments.Feb 1 2017, 11:40 AM
171 ↗(On Diff #86715)

No, because we overwrite it unconditionally here.

zturner edited edge metadata.Feb 1 2017, 11:51 AM

Hmm, that's a bummer. In that case I guess I like using the Optional<> better. More comments inline.

171 ↗(On Diff #86715)

Before this, add assert(!LastError.hasValue());

183 ↗(On Diff #86715)

Then this can change to

Error Result = std::move(*LastError);
return std::move(Result);
ruiu updated this revision to Diff 86715.Feb 1 2017, 2:17 PM
  • Use llvm::Optional<Error> instead of Error.
zturner accepted this revision.Feb 1 2017, 2:18 PM
This revision is now accepted and ready to land.Feb 1 2017, 2:18 PM
This revision was automatically updated to reflect the committed changes.