This is an archive of the discontinued LLVM Phabricator instance.

raw_ostream: add operator<< overload for std::error_code
ClosedPublic

Authored by labath on Aug 2 2019, 2:52 AM.

Details

Summary

The main motivation for this is unit tests, which contain a large macro
for pretty-printing std::error_code, and this macro is duplicated in
every file that needs to do this. However, the functionality may be
useful elsewhere too.

In this patch I have reimplemented the existing ASSERT_NO_ERROR macros
to reuse the new functionality, but I have kept the macro (as a
one-liner) as it is slightly more readable than ASSERT_EQ(...,
std::error_code()).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Aug 2 2019, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 2:52 AM

I'm lacking a bit of context here.
Why do we prefer this to EXPECT_FALSE(err) /EXPECT_EQ(err, std::errc_address_in_use) with an appropriate value printer for std::error_code?

Apart from being a bit more obscure, here the Failed() matcher doesn't provide any overload to specify the code, so this api seems to encourage people not to specify the code even if they know it.

labath added a comment.Aug 2 2019, 4:04 AM

I'm lacking a bit of context here.
Why do we prefer this to EXPECT_FALSE(err) /EXPECT_EQ(err, std::errc_address_in_use) with an appropriate value printer for std::error_code?

Apart from being a bit more obscure, here the Failed() matcher doesn't provide any overload to specify the code, so this api seems to encourage people not to specify the code even if they know it.

That's a good point. There wasn't a huge deal of thought going into this. I automatically reached for the Succeded/Failed matchers because:
a) I was involved in creating them
b) it seemed nice to have a single entity which can match all kinds of errors

(a) is not much of a reason but I think (b) has some value to it. Should the need arise we can also teach it to match ErrorOr<T>, and we can also come up with a way to match error codes or categories similar to how you can now say Failed<InfoT>() to match llvm::Errors of specific types.

OTOH, I agree that it would be nice if std::error_code came out "right" no matter how it ended up being used in the assertions. Then these matchers would be a purely optional add-on that one can use when it suits him. However, I am not sure if that is actually achievable. If I know my googletest correctly, then both operator<< and the PrintTo function need to be defined in the same namespace as the object they are printing, which in this case would be std...

labath added a comment.Aug 2 2019, 4:21 AM

Hmm... so, by piggy-backing on the StreamSwitch thingy I was able to write a operator<<(raw_ostream, std::error_code) which would print the error message. However, that only seems to work when comparing two error_codes (EXPECT_EQ and friends). When simply testing the truth value (EXPECT_FALSE), it seems to get converted to bool before it gets printed and all I get is the signature "false is not true" :P. Maybe we could adopt the convention to write EXPECT_EQ(foo(), std::error_code())? -- I find it confusing that EXPECT_FALSE expects success anyway...

labath added a comment.Aug 6 2019, 4:07 AM

@sammccall, what do you think of the operator<<(raw_ostream&, std::error_code) idea? Is that the direction I should go in? If so, where would the implementation of that function live? (next to raw_ostream, somewhere under llvm/Testing, or under gtest/internal/custom)

@sammccall, what do you think of the operator<<(raw_ostream&, std::error_code) idea? Is that the direction I should go in? If so, where would the implementation of that function live? (next to raw_ostream, somewhere under llvm/Testing, or under gtest/internal/custom)

Sorry about taking a while to get back to this.
So I think we should have operator<< no matter what: even if we go with ASSERT_THAT(x, Succeeded()) being able to print x still improves the output.
And it'll make std::error_code work with llvm::errs() << and formatv and so on.
I think it's fine to put it in raw_ostream.h along with the others - this isn't test-specific. We might need an overload for std::errc too?

Generally, I'd rather have the "plain" styles of assertions if the readability + errors are as good, since there's less to learn.
We still have a lot of ASSERT_TRUE(X == Y) in llvm! I think it's important this stuff work as well as possible without special knowledge.
Failed() and Succeeded() seem mostly useful when wrapping values (ErrorOr --> Failed(some_error_matcher) or Succeeded(some_value_matcher)) or to deal with the terror that is llvm::Error move-semantics. But I don't feel strongly here, if you'd like to be able to use Failed() with plain std::error_code, that seems fine in Testing/Error.h, though I probably won't use it myself.

labath updated this revision to Diff 214588.Aug 12 2019, 1:50 AM

Thanks for the feedback. I've redone the patch to add a std::error_code overload
to llvm::raw_ostream. This makes using std::error_code in tests "almost just
work", with the "almost" being there due to the ASSERT_TRUE problem.

I've dropped the matcher part of the patch because the need for it largely
disappears with this approach.

labath retitled this revision from unittests: Add a (centralized) ability to match std::error_code to raw_ostream: add operator<< overload for std::error_code.Aug 12 2019, 1:53 AM
labath edited the summary of this revision. (Show Details)
sammccall accepted this revision.Aug 12 2019, 5:57 AM

Thanks! I actually like the ASSERT_NO_ERROR macros, std::error_code() meaning success is pretty obscure to write everywhere.

lib/Support/raw_ostream.cpp
143 ↗(On Diff #214588)

Looks like typical output is system:2 address already in use

This reads a little oddly to me: the colon is easily mistaken for (english) punctuation, but that leads to misreading the text. And the category/number are first, but I think usually less relevant than the description.

Maybe consider address already in use (system:2) or so?

This revision is now accepted and ready to land.Aug 12 2019, 5:57 AM
labath marked 2 inline comments as done.Aug 14 2019, 6:29 AM

Thanks! I actually like the ASSERT_NO_ERROR macros, std::error_code() meaning success is pretty obscure to write everywhere.

Yeah, I see what you mean. The thing that really annoyed me was pasting the large macro everywhere, but a one-liner is not that much of a big deal.

lib/Support/raw_ostream.cpp
143 ↗(On Diff #214588)

Done.

labath edited the summary of this revision. (Show Details)Aug 14 2019, 6:31 AM
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.

Unfortunately, I've hit a snag with this implementation. It turns out changing bool(EC) to EC != std::error_code() is not NFC because one can have two "success" values that still compare as unequal (e.g. std::error_code(0, generic_category()) and std::error_code(0, system_category()). This is a problem because std::error_code() is std::error_code(0, system_category()) and there some (though few) places in llvm that do return std::error_code(errno, generic_category()), and at least some of those places are not bugs (though I think some are).

So I don't think we can go for the EXPECT_EQ(EC, std::error_code()) option. This unfortunately brings us back to square one. I think the only way to reasonably implement this is to go the matcher route, as that is the only way (that I know of) we can call operator bool to properly determine success-ness of the value, while still being able to print the exact failure message. (Though we can keep the operator<< overload, so that matching direct matching against a known error code still works.)

What do you think ?