Skip to content

Commit

Permalink
Fix parentheses removal for XLS_ASSIGN_OR_RETURN() macro.
Browse files Browse the repository at this point in the history
`XLS_ASSIGN_OR_RETURN((std::tuple<int, int> some_tuple), SomeFunction())` needs the wrappinng parentheses around `some_tuple` because it contains a comma, which would otherwise be interpreted as multiple macro arguments. Internally, the macro checks if the first argument is wrapped in parentheses and removes them if present.

Unfortunately, the macro that checks for wrapping parentheses fails to correctly identify parentheses under gcc, which then results in compilation failures because a declaration is wrapped in parentheses, e.g. `(std::tuple<int, int> a_tuple) = some_value`.

This change switches to using `__VA_OPT__(,)` instead of relying on token-pasting behavior, which evidently does not match between gcc and clang. This change also re-enables tests on gcc now that they pass.

Tested against gcc Debian 12.2.0-10.

PiperOrigin-RevId: 600930055
  • Loading branch information
grebe authored and copybara-github committed Jan 23, 2024
1 parent a2de376 commit ce8714a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 14 deletions.
15 changes: 7 additions & 8 deletions xls/common/status/status_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,14 @@
#define XLS_STATUS_MACROS_IMPL_REM(...) __VA_ARGS__
#define XLS_STATUS_MACROS_IMPL_EMPTY()

// Internal helpers for emptyness arguments check.
#define XLS_STATUS_MACROS_IMPL_IS_EMPTY_INNER(...) \
XLS_STATUS_MACROS_IMPL_IS_EMPTY_INNER_I(__VA_ARGS__, 0, 1)
#define XLS_STATUS_MACROS_IMPL_IS_EMPTY_INNER_I(e0, e1, is_empty, ...) is_empty

// __VA_OPT__ expands to nothing if __VA_ARGS__ are empty, and otherwise expands
// to its argument. We use __VA_OPT__ here to expand to true if __VA_ARGS__ is
// empty and false otherwise- the `EMPTY_I` helper macro expands to the first
// argument.
#define XLS_STATUS_MACROS_IMPL_IS_EMPTY(...) \
XLS_STATUS_MACROS_IMPL_IS_EMPTY_I(__VA_ARGS__)
#define XLS_STATUS_MACROS_IMPL_IS_EMPTY_I(...) \
XLS_STATUS_MACROS_IMPL_IS_EMPTY_INNER(_, ##__VA_ARGS__)
XLS_STATUS_MACROS_IMPL_IS_EMPTY_I(__VA_OPT__(0, )1)
#define XLS_STATUS_MACROS_IMPL_IS_EMPTY_I(is_empty, ...) is_empty


// Internal helpers for if statement.
#define XLS_STATUS_MACROS_IMPL_IF_1(_Then, _Else) _Then
Expand Down
6 changes: 0 additions & 6 deletions xls/common/status/status_macros_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ TEST(AssignOrReturn, Works) {
EXPECT_THAT(func().message(), Eq("EXPECTED"));
}

// Note: GCC (as of 9.2.1) doesn't seem to support this trick.
#ifdef __clang__
TEST(AssignOrReturn, WorksWithCommasInType) {
auto func = []() -> absl::Status {
XLS_ASSIGN_OR_RETURN((std::tuple<int, int> t1),
Expand Down Expand Up @@ -121,7 +119,6 @@ TEST(AssignOrReturn, WorksWithStructureBindings) {

EXPECT_THAT(func().message(), Eq("EXPECTED"));
}
#endif

TEST(AssignOrReturn, WorksWithParenthesesAndDereference) {
auto func = []() -> absl::Status {
Expand Down Expand Up @@ -185,8 +182,6 @@ TEST(AssignOrReturn, WorksWithAdaptorFunc) {
AllOf(HasSubstr("EXPECTED A"), HasSubstr("EXPECTED B")));
}

// Note: GCC (as of 9.2.1) doesn't seem to support this trick.
#ifdef __clang__
TEST(AssignOrReturn, WorksWithThirdArgumentAndCommas) {
auto fail_test_if_called = [](xabsl::StatusBuilder builder) {
ADD_FAILURE();
Expand Down Expand Up @@ -215,7 +210,6 @@ TEST(AssignOrReturn, WorksWithThirdArgumentAndCommas) {
EXPECT_THAT(func().message(),
AllOf(HasSubstr("EXPECTED A"), HasSubstr("EXPECTED B")));
}
#endif

TEST(AssignOrReturn, WorksWithAppendIncludingLocals) {
auto func = [&](const std::string& str) -> absl::Status {
Expand Down

0 comments on commit ce8714a

Please sign in to comment.