Skip to content

refactor(bvar): expand metric constructor macro#3400

Open
darion-yaphet wants to merge 1 commit into
apache:masterfrom
darion-yaphet:modernize-bvar-constructors
Open

refactor(bvar): expand metric constructor macro#3400
darion-yaphet wants to merge 1 commit into
apache:masterfrom
darion-yaphet:modernize-bvar-constructors

Conversation

@darion-yaphet

Copy link
Copy Markdown
Contributor

Summary

  • Remove the COMMON_VARIABLE_CONSTRUCTOR macro
  • Replace its uses in the Babylon counter variants of Adder, Maxer, Miner, and IntRecorder with explicit constructors
  • Preserve the existing constructor behavior for default construction, expose(name), and expose_as(prefix, name)

Motivation

COMMON_VARIABLE_CONSTRUCTOR hid public constructors behind macro expansion, making the code harder to read, search, and navigate with regular C++ tooling.

This change keeps the API behavior unchanged while making the constructors visible at their declaration sites.

using Base::Base was intentionally avoided because it would expose additional internal base constructors in the Babylon counter path and could broaden the public
API.

Testing

  • cmake --build build --target brpc-static -j6
  • git diff --check
  • rg COMMON_VARIABLE_CONSTRUCTOR . -g '!build/**'

Notes

The WITH_BABYLON_COUNTER path was not compiled locally because the available Bazel setup is missing the repository-required Bazel 7.2.1 binary.

Inline default/name/prefix constructors in Adder, Maxer, Miner, and
IntRecorder, and drop the shared macro from variable.h.

Co-authored-by: Cursor <[email protected]>

@chenBright chenBright 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.

I think this is to simplify the code, which is fine, like the DISALLOW_COPY_AND_ASSIGN macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants