[Impeller] added static check that fragment shader and vertex shaders slots match #52174
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
impeller/renderer/pipeline.h
Outdated
| /// match the output slots of the vertex shaders. | ||
| /// It's not used at runtime. | ||
| template <typename VertexShaderT, typename FragmentShaderT> | ||
| class ShaderBindingComparator { |
There was a problem hiding this comment.
This is a class because it's required for partial specialization. Also I wish we got better feedback when this failed, but I couldn't figure out how to get multiple static asserts happening in the loop, that's why it returns bool.
impeller/renderer/pipeline.h
Outdated
| struct ClipVertexShader; | ||
|
|
||
| template <typename FragmentShaderT> | ||
| class ShaderBindingComparator<CheckerboardVertexShader, FragmentShaderT> { |
There was a problem hiding this comment.
Another idea for if you don't want to update the code gen template, you could have a third template size_t argument that is the count of the array elements and have a specialization for zero that is true.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
LGTM with nits. I'd rather we patch the codegen template so the checkerboard and clip shaders aren't specialized here.
|
This is awesome. Moving more stuff to be compile time verified. |
…ex shaders slots match (flutter/engine#52174)
…146898) flutter/engine@25b09e8...818191d 2024-04-17 [email protected] [Impeller] added static check that fragment shader and vertex shaders slots match (flutter/engine#52174) 2024-04-17 [email protected] [macOS] FlutterSurfaceManager should not return surfaces that are in use (flutter/engine#52082) 2024-04-17 [email protected] [macOS] FlutterView should not override platform view cursor (flutter/engine#52159) 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
…lutter#146898) flutter/engine@25b09e8...818191d 2024-04-17 [email protected] [Impeller] added static check that fragment shader and vertex shaders slots match (flutter/engine#52174) 2024-04-17 [email protected] [macOS] FlutterSurfaceManager should not return surfaces that are in use (flutter/engine#52082) 2024-04-17 [email protected] [macOS] FlutterView should not override platform view cursor (flutter/engine#52159) 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
This comment was marked as spam.
This comment was marked as spam.
3 participants
fixes flutter/flutter#146452
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.