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

Handle removed shaders more gracefully in malioc_diff.py - #40720

Merged
auto-submit[bot] merged 1 commit into
flutter-team-archive:mainfrom
zanderso:malioc-diff-shader-rm
Mar 28, 2023
Merged

auto-submit[bot] merged 1 commit into
flutter-team-archive:mainfrom
zanderso:malioc-diff-shader-rm

Conversation

@zanderso

Copy link
Copy Markdown
Contributor

No description provided.

@bdero bdero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@zanderso zanderso added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 28, 2023
@bdero

bdero commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Instead of doing partial diffs on CI, would it make sense to just generate a blessed file and literally run diff ./impeller/tools/malioc.json [after.json]?

@auto-submit
auto-submit Bot merged commit 9025d98 into flutter-team-archive:main Mar 28, 2023
@zanderso
zanderso deleted the malioc-diff-shader-rm branch March 28, 2023 19:42
@zanderso

Copy link
Copy Markdown
Contributor Author

Instead of doing partial diffs on CI, would it make sense to just generate a blessed file and literally run diff ./impeller/tools/malioc.json [after.json]?

Yeah, there are a couple of different directions we could go. One thought I had was that when there is a change in performance in an existing shader, then having the prettier message is useful. But if that's not actually the case, then we can tear out a bunch of code. OTOH, if it is useful, then I'd go even further in the same direction and pull the human readable display names and verbose descriptions out of the malioc json output as well.

@bdero

bdero commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Having a readable interpretation seems valuable to me. Perhaps we could do both and get the best of both worlds. The primary value in printing the full diff for me would be the ability to just copy it from CI and apply it using a patch tool in cases where I forgot (or can't) run malioc locally.

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

Development

Successfully merging this pull request may close these issues.

2 participants