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

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Jan 16, 2024

The EXPECT_CALL_IS_EVENT macro used features that are not supported by Visual Studio 2022's intellisense, which results in >130 errors when editing in Visual Studio. These issues only affect the editing experience, building still works as expected.

This change reduces false errors in Visual Studio by making EXPECT_CALL_IS_EVENT buildable in Visual Studio.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is [test-exexpt]. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.


#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

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.

.physical = _physical, \
.logical = _logical, \
.character = _character, \
.synthesized = _synthesized, \
Copy link
Member Author

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

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

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

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.

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 17, 2024
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

The changes seem fine if they're necessary, but curious why they're necessary since the engine/embedder are built with clang, not MSVC.

@loic-sharma
Copy link
Member Author

The changes seem fine if they're necessary, but curious why they're necessary since the engine/embedder are built with clang, not MSVC.

Summarizing our Discord conversation: Visual Studio's Intellisense appears to use MSVC. Building within Visual Studio still worked as expected, this change only improves the editor experience.

@auto-submit auto-submit bot merged commit 6614de3 into flutter:main Jan 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 18, 2024
…141744)

flutter/engine@73a2de5...98c16b4

2024-01-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from d6c08fa9bb54 to 6ff69d6b7f59 (15 revisions)" (flutter/engine#49852)
2024-01-18 [email protected] Roll Skia from 5abf9717ea92 to 31f275e8fcc7 (8 revisions) (flutter/engine#49850)
2024-01-18 [email protected] Reland: [Impeller] Turned on new blur. (#48472) (flutter/engine#49642)
2024-01-17 [email protected] [Windows] Reduce Visual Studio build errors caused by keyboard unit tests (flutter/engine#49814)
2024-01-17 [email protected] [Impeller] disabled misleading vulkan golden image tests (flutter/engine#49836)
2024-01-17 [email protected] [Windows] Remove unnecessary statics in keyboard (flutter/engine#49834)
2024-01-17 [email protected] Roll Skia from 5c9e3474cf13 to 5abf9717ea92 (1 revision) (flutter/engine#49831)
2024-01-17 [email protected] Move mac cache builder to bringup. (flutter/engine#49843)
2024-01-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Switch from transient stencil-only to depth+stencil buffer." (flutter/engine#49832)
2024-01-17 [email protected] Roll Skia from d8da1ee69767 to 5c9e3474cf13 (2 revisions) (flutter/engine#49827)
2024-01-17 [email protected] Manual roll Dart SDK from d6c08fa9bb54 to 6ff69d6b7f59 (15 revisions) (flutter/engine#49825)
2024-01-17 [email protected] Roll Skia from 02e94b3b4d29 to d8da1ee69767 (1 revision) (flutter/engine#49824)
2024-01-17 [email protected] Flutter GPU: Add GpuContext.createHostBuffer (flutter/engine#49822)
2024-01-17 [email protected] Roll Fuchsia Linux SDK from Klxww53tA4-TG5pA9... to GuU0e5WxJCi92Scz8... (flutter/engine#49823)
2024-01-17 [email protected] [Impeller] Switch from transient stencil-only to depth+stencil buffer. (flutter/engine#47987)
2024-01-17 [email protected] Roll Skia from 31309ff09537 to 02e94b3b4d29 (43 revisions) (flutter/engine#49819)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Klxww53tA4-T to GuU0e5WxJCi9

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@loic-sharma loic-sharma deleted the vs_keyboard_errors branch January 18, 2024 18:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects: desktop autosubmit Merge PR when tree becomes green via auto submit App platform-windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants