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

Conversation

@jonahwilliams
Copy link
Contributor

mostly removing unused fml/macros, but a few others that clangd surfaced. This compiled locally,hope it works on CI!

@jonahwilliams jonahwilliams marked this pull request as ready for review May 6, 2024 18:44
@jonahwilliams jonahwilliams requested a review from matanlurey May 6, 2024 18:45
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

Neat, gets us closer to IWYU.

You might want to add test: all and push a dummy commit to run the full clangd on pre-sub.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Are these removed because they aren't used or because they are brought in transitively? The former is good, the latter would be a violation of iwyu. The iwyu tool can be used to identify these as well.

@jonahwilliams jonahwilliams added the test: all See https://github.com/flutter/engine/blob/main/docs/ci/Engine-pre-submits-and-post-submits.md label May 6, 2024
@jonahwilliams
Copy link
Contributor Author

fml/macros are unused everywhere. For everything else, if clangd highlighted it as unused I removed it, unless I knew it was special like build config.h or other macros

The former is good, the latter would be a violation of iwyu. The iwyu tool can be used to identify these as well.

This isn't an attempt to guess what IWYU would do. We can't use IWYU anyway, unless we fix all of our forward declares.

@jonahwilliams
Copy link
Contributor Author

In general though, clangd seems to have some similar logic? If you are including a header to get to a transitive header, it will tell you the first header is unused. Then when you remove it and get an include error, the suggested include is often the non-transitive header

@gaaclarke
Copy link
Member

This isn't an attempt to guess what IWYU would do. We can't use IWYU anyway, unless we fix all of our forward declares.

FWIW I've had success using it locally when splitting up the aiks tests. It may not be easy to setup on ci but for local cleanups it was helpful.

@matanlurey
Copy link
Contributor

Just a clarification:

@jonahwilliams: We can't use IWYU anyway, unless we fix all of our forward declares.

We can, https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-keep.

@jonahwilliams
Copy link
Contributor Author

oh TIL!

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2024
@auto-submit auto-submit bot merged commit 470d16b into flutter:main May 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 6, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 6, 2024
…147895)

flutter/engine@463ff7d...422f92b

2024-05-06 [email protected] Migrate third_party/android_tools to flutter/third_party (flutter/engine#52582)
2024-05-06 [email protected] Roll Skia from a5c042cb4b12 to 869cacf2a3f1 (1 revision) (flutter/engine#52583)
2024-05-06 [email protected] [Impeller] remove unused includes. (flutter/engine#52579)
2024-05-06 [email protected] Roll Skia from da772ace76ff to a5c042cb4b12 (6 revisions) (flutter/engine#52581)
2024-05-06 [email protected] [Impeller] fix missing GPU probe. (flutter/engine#52580)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller test: all See https://github.com/flutter/engine/blob/main/docs/ci/Engine-pre-submits-and-post-submits.md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants