-
Notifications
You must be signed in to change notification settings - Fork 6k
Rename instrumentation to stopwatch
#45196
Rename instrumentation to stopwatch
#45196
Conversation
| #include "flutter/flow/instrumentation.h" | ||
| #include "gmock/gmock.h" | ||
| #include "flutter/flow/stopwatch.h" | ||
| #include "gmock/gmock.h" // IWYU pragma: keep |
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.
@dnfield was the issue that IWYU didnt understand all of the gn options and so tests didn't work well with it?
Anyway, unsure if we want to start adding IWYU pragmas until we understand how many we'd need
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.
Ack. This was more of a "hmm, what's going on here".
From my understanding, gmock specifically needs a pragma (I see the Chromium codebase doing the same thing).
I'm going to merge, but if we end up wanting to remove this I have no qualms.
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.
SGTM
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.
its its only for gmock and only for a few places I think thats fine. I just dont want IWYU pragmas on half of our includes 😄
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.
Ack. I'll run that experiment later this week to see how bad it would be.
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 do not think we're using IWYU , are we?
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 was asking about the previous issue with trying to use IWYU that you mentioned.
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.
Ahh I can't remember why now, I think there was some challenge getting it set up to run period, not so much with these details. Apparently there's a clangtidy proposal for it too.
jonahwilliams
left a comment
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 w/ question
|
Merging ignoring |
…133523) flutter/engine@1e82196...ffda7e3 2023-08-29 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from u44zzvYd85WSDMpS3... to 68DESTl_q6I45UJyb... (flutter/engine#45197) 2023-08-29 matanlurey@users.noreply.github.com Rename `instrumentation` to `stopwatch` (flutter/engine#45196) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from u44zzvYd85WS to 68DESTl_q6I4 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 jimgraham@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
... since that's what the code is/does. Partial work towards flutter/flutter#126009.
... since that's what the code is/does.
Partial work towards flutter/flutter#126009.