This is an archive of the discontinued LLVM Phabricator instance.

Add FIXMEs to all derived classes of std::error_category.
ClosedPublic

Authored by pcc on May 24 2016, 12:47 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 58294.May 24 2016, 12:47 PM
pcc retitled this revision from to Add FIXMEs to all derived classes of std::error_category..
pcc updated this object.
pcc added a reviewer: rafael.
pcc added a subscriber: llvm-commits.
rafael edited edge metadata.May 24 2016, 12:54 PM
rafael added a subscriber: rafael.

LGTM, thanks!

lhames added a subscriber: lhames.May 24 2016, 12:57 PM

I like the idea of making it clearer that we're moving to Error as the standard error handling mechanism, but this could generate confusion because (at least the way I read them) these FIXMEs suggest that we could remove the error categories piecemeal, which isn't the case: To maintain compatibility with std::error_code (via the errorToErrorCode and expectedToErrorOr methods) we require that all error types be convertible to an equivalent error_code. We can't remove that requirement until all LLVM code has moved over to Error.

Oh - I see this was motivated by http://reviews.llvm.org/D20550.

Maybe a re-worded FIXME would work:

// FIXME: This class is only here to support the transition to llvm::Error. It will be
// removed once this transition is complete. Clients should prefer to deal with the
// Error value directly, rather than converting to error_code.

It is a bit more verbose though.

pcc added a comment.May 24 2016, 1:16 PM

Maybe a re-worded FIXME would work:

SGTM, I'll commit with your proposed wording.

lhames accepted this revision.May 24 2016, 1:19 PM
lhames added a reviewer: lhames.

Cool. Thanks Peter!

This revision is now accepted and ready to land.May 24 2016, 1:19 PM
This revision was automatically updated to reflect the committed changes.