Skip to content
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

[workspace] Patch googlebenchmark to avoid shadow warnings #19796

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jul 16, 2023

This allows us to remove copy-pasta in every benchmark we write.

+@rpoyner-tri for feature review, please.

\CC @RussTedrake whichever #19771 or this merges first, the other one should rebase to delete the pasta in the new benchmark, too.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: medium release notes: fix This pull request contains fixes (no new features) labels Jul 16, 2023
@RussTedrake RussTedrake self-assigned this Jul 16, 2023
Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

+@RussTedrake for feature review, if you like?
:lgtm:

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform)


systems/benchmarking/multilayer_perceptron_benchmark.cc line 27 at r1 (raw file):

  }

  void SetUp(benchmark::State& state) {  // NOLINT(runtime/references)

btw -- apparently switching to BenchmarkStateRef is another approach (used above) to trick the linter? Perhaps we should make them consistent?

This allows us to remove copy-pasta in every benchmark we write.
@jwnimmer-tri jwnimmer-tri force-pushed the googlebenchmark-shadow-pasta branch from c01a517 to bf56cd0 Compare July 16, 2023 19:50
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Thanks @RussTedrake; I'll count you as platform review. I'd still like Rico to feature-review as our subject matter expert.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform)


systems/benchmarking/multilayer_perceptron_benchmark.cc line 27 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- apparently switching to BenchmarkStateRef is another approach (used above) to trick the linter? Perhaps we should make them consistent?

Done

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),RussTedrake(platform)

@jwnimmer-tri jwnimmer-tri merged commit 7cb3445 into RobotLocomotion:master Jul 16, 2023
@jwnimmer-tri jwnimmer-tri deleted the googlebenchmark-shadow-pasta branch July 16, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants