Page MenuHomePhabricator

[Support] Introduce createStringError helper function
ClosedPublic

Authored by vleschuk on Jul 25 2018, 3:51 PM.

Details

Summary

The function in question is copy-pasted lots of times in DWARF-related classes. Thus it will make sense to place its implementation into the Support library.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk created this revision.Jul 25 2018, 3:51 PM

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

Do you mean something like that the following?

logAllUnhandledErrors(createStringError("bar"), S, "");                         
EXPECT_EQ(S.str(), "bar\n") ;

I can add it but it will result in compiler warnings "warning: format string is not a string literal (potentially insecure) [-Wformat-security]" which is not looking good. We can temporary disable warnings for this test using compiler-specific pragmas but it will be very messy...

lhames added inline comments.Jul 25 2018, 4:34 PM
include/llvm/Support/Error.h
1125–1133 ↗(On Diff #157382)

For now I think this needs to take the std::error_code for the StringError as an argument too, rather than using inconvertibleErrorCode().

The reason for this is that using inconvertibleErrorCode breaks Error / std::error_code interoperability: any attempt to convert an Error value constructed with an inconvertibleErrorCode triggers an immediate abort. It is meant to be used explicitly, and only for code that explicitly documents that it does not support interoperability. It should not be default behavior.

Given the increasingly wide adoption of Error we could talk about changing this policy, but that is a discussion we should have on the dev list where it will reach a wide audience.

vleschuk updated this revision to Diff 157407.Jul 25 2018, 5:22 PM
vleschuk marked an inline comment as done.

Added explicit std::error_code param to createStringError() function.

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

Do you mean something like that the following?

logAllUnhandledErrors(createStringError("bar"), S, "");                         
EXPECT_EQ(S.str(), "bar\n") ;

I can add it but it will result in compiler warnings "warning: format string is not a string literal (potentially insecure) [-Wformat-security]" which is not looking good. We can temporary disable warnings for this test using compiler-specific pragmas but it will be very messy...

You can add an overload for the degenerate case to fix this:

Error createStringError(std::error_code EC, char const *Msg) {
  return make_error<StringError>(Msg, EC);
}
vleschuk updated this revision to Diff 157413.Jul 25 2018, 5:59 PM

Added overloaded version of createStringError() function which is not variadic, also added corresponding test.

lhames accepted this revision.Jul 25 2018, 7:14 PM

Cool. Looks good to me. Thanks Victor!

This revision is now accepted and ready to land.Jul 25 2018, 7:14 PM
This revision was automatically updated to reflect the committed changes.

Lang's suggestion was indeed what I had in mind. Thanks!