-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix clang tidy error and crash #42564
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hold on this until we see whether the rebase on #42399 helps?
As for the crasher in clang-tidy, @knopp if you can post more details here or in a new GitHub issue, I can try to raise visibility on that with a toolchain team. |
I don't have much regarding the crash. I can reproduce it locally on main
Stacktrace
By experimenting a bit I found out that the crash doesn't happen when the assignment to |
Btw., I think it also crashes my |
@@ -64,8 +64,13 @@ | |||
|
|||
auto des = [[MTLDepthStencilDescriptor alloc] init]; | |||
|
|||
des.depthCompareFunction = ToMTLCompareFunction(depth->depth_compare); | |||
des.depthWriteEnabled = depth->depth_write_enabled; | |||
// These temporary variables are necessary for clang-tidy (Fuchsia LLVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the crash that this fixes happening on CI. Could we maybe split this out into a separate issue/PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, sorry. I see it now.
…128271) flutter/engine@f9f7238...7f12e34 2023-06-05 matej.knopp@gmail.com Fix clang tidy error and crash (flutter/engine#42564) 2023-06-05 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from gp1k1agqxQIz0oTXV... to vAwdggqMrL1yoH_Zn... (flutter/engine#42570) 2023-06-05 zanderso@users.noreply.github.com Revert Dart SDK to 3.1.0-163.0.dev for branch alignment (flutter/engine#42569) 2023-06-05 zanderso@users.noreply.github.com Check more optional accesses (flutter/engine#42528) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from gp1k1agqxQIz to vAwdggqMrL1y 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Clang tidy checks for #42399 are failing for
formats_mtl.mm
andcontext_mtl.mm
. Weirdly this happens even though none of these files where modified in the branch.For
context_mtl.mm
the fix is straightforward, just a missingstd::move
.For
formats_mtl.mm
clang-tidy actually crashes on the assignments todes.depthCompareFunction
anddes.depth_write_enabled
. The crash doesn't happen if the assignment is done through a temporary variable.I'm not sure if we should have a workaround for the crash or disable clang-tidy on the file completely (FLUTTER_NOLINT).
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.