Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Windows] Reduce Visual Studio build errors caused by keyboard unit tests #49814

Merged
auto-submit merged 1 commit into from
Jan 17, 2024
Merged

[Windows] Reduce Visual Studio build errors caused by keyboard unit tests #49814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions shell/platform/windows/keyboard_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,11 @@ class KeyboardTest : public WindowsTest {
// Define compound `expect` in macros. If they're defined in functions, the
// stacktrace wouldn't print where the function is called in the unit tests.

#define EXPECT_CALL_IS_EVENT(_key_call, ...) \
EXPECT_EQ(_key_call.type, KeyCall::kKeyCallOnKey); \
EXPECT_EVENT_EQUALS(_key_call.key_event, __VA_ARGS__);
Copy link
Member Author

@loic-sharma loic-sharma Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC (Visual Studio's C++ compiler) doesn't expand __VA_ARGS__ well. Instead of "expanding" the __VA_ARGS into multiple arguments, it places them all into a single argument. This causes EXPECT_EVENT_EQUALS to have unexpected commas while in Visual Studio.

#define EXPECT_CALL_IS_EVENT(_key_call, _type, _physical, _logical, \
_character, _synthesized) \
EXPECT_EQ(_key_call.type, KeyCall::kKeyCallOnKey); \
EXPECT_EVENT_EQUALS(_key_call.key_event, _type, _physical, _logical, \
_character, _synthesized);

#define EXPECT_CALL_IS_TEXT(_key_call, u16_string) \
EXPECT_EQ(_key_call.type, KeyCall::kKeyCallOnText); \
Expand Down
20 changes: 12 additions & 8 deletions shell/platform/windows/testing/test_keyboard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,13 +145,17 @@ class MockMessageQueue {
// Expect the |_target| FlutterKeyEvent has the required properties.
#define EXPECT_EVENT_EQUALS(_target, _type, _physical, _logical, _character, \
_synthesized) \
EXPECT_PRED_FORMAT2(_EventEquals, _target, \
(FlutterKeyEvent{ \
.type = _type, \
.physical = _physical, \
.logical = _logical, \
.character = _character, \
.synthesized = _synthesized, \
Copy link
Member Author

@loic-sharma loic-sharma Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This avoids C++20 designated initializers as Visual Studio reports these as build errors.

Copy link
Contributor

@dkwingsmt dkwingsmt Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awful since I've been using it all over the place in engine...

Copy link
Member

@cbracken cbracken Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're building the engine/embedder with clang, so I'd have thought this shouldn't be an issue. This definitely used to be an issue when building with MSVC though, so we can't use it for example in the runner.

Has something regressed?

Copy link
Member Author

@loic-sharma loic-sharma Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has something regressed?

No, this isn't a regression. Building inside of Visual Studio still works, however, the Visual Studio editor reports errors.

I would say it's fine to keep using designated initializers in C++ code. This particular macro is a bit of an exception as it's used pervasively in keyboard_unittests, resulting in massive amounts of errors.

}));
EXPECT_PRED_FORMAT2( \
_EventEquals, _target, \
(FlutterKeyEvent{ \
/* struct_size = */ sizeof(FlutterKeyEvent), \
/* timestamp = */ 0, \
/* type = */ _type, \
/* physical = */ _physical, \
/* logical = */ _logical, \
/* character = */ _character, \
/* synthesized = */ _synthesized, \
/* device_type = */ kFlutterKeyEventDeviceTypeKeyboard, \
}));

#endif // FLUTTER_SHELL_PLATFORM_WINDOWS_TESTING_TEST_KEYBOARD_H_