Skip to content

refactor(butil): replace integer string conversions - #3534

Open
darion-yaphet wants to merge 1 commit into
apache:masterfrom
darion-yaphet:refactor/std-to-string-integer
Open

refactor(butil): replace integer string conversions#3534
darion-yaphet wants to merge 1 commit into
apache:masterfrom
darion-yaphet:refactor/std-to-string-integer

Conversation

@darion-yaphet

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:

Replace internal uses of legacy butil integer-to-string helpers with std::to_string to reduce duplicated conversion logic.

What is changed and the side effects?

Changed:

  • Use std::to_string for internal IntToString and UintToString call sites.
  • Keep the public butil::IntToString(int) and butil::UintToString(unsigned) APIs as compatibility wrappers backed by std::to_string.
  • Update integer conversion tests while preserving unsigned conversion behavior for negative int inputs.

Side effects:

  • Performance effects: No benchmark was run; no expected material impact.

  • Breaking backward compatibility: No. The existing public helper APIs remain available.


Check List:

  • Please make sure your changes are compilable.
    • cmake --build build --target BUTIL_LIB -j6
    • cmake --build build --target test_butil -j6
  • Related tests updated and passed.
    • StringNumberConversionsTest.IntToString
    • HttpTest.http_head
  • Please follow Contributor Covenant Code of Conduct.

@darion-yaphet

Copy link
Copy Markdown
Contributor Author

IntToString and UintToString were not removed directly because they are publicly exposed butil APIs. Removing them directly would cause external BRPC users to fail during compilation or linking.

Copilot AI 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.

🟡 Changes recommended

The updated StringNumberConversionsTest.IntToString no longer exercises the compatibility wrappers (butil::IntToString/butil::UintToString), reducing coverage for the APIs explicitly kept by this PR.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors integer-to-string conversions across butil and related call sites by replacing internal uses of butil::{Int,Uint}ToString with std::to_string, while keeping the public wrapper APIs for compatibility.

Changes:

  • Replace internal call sites of legacy integer conversion helpers with std::to_string.
  • Re-implement butil::IntToString(int) and butil::UintToString(unsigned) as thin wrappers over std::to_string.
  • Update affected unit tests and examples to use std::to_string at call sites.
File summaries
File Description
test/string_number_conversions_unittest.cc Adjusts integer conversion expectations (currently switches to std::to_string in the IntToString test).
test/simple_thread_unittest.cc Uses std::to_string when composing thread names; drops dependency on string_number_conversions.
test/shared_memory_unittest.cc Uses std::to_string in shared memory name construction.
test/brpc_http_rpc_protocol_unittest.cpp Uses std::to_string for HTTP header value formatting.
src/butil/version.cc Replaces IntToString with std::to_string in version parsing/stringification logic.
src/butil/threading/simple_thread.cc Uses std::to_string for thread ID formatting in SimpleThread::ThreadMain.
src/butil/strings/string_number_conversions.cc Re-implements IntToString/UintToString wrappers via std::to_string.
src/butil/location.cc Uses std::to_string for line number formatting in Location::ToString.
example/baidu_proxy_and_generic_call/proxy.cpp Uses std::to_string for response user fields error codes.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/string_number_conversions_unittest.cc Outdated

Copilot AI 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.

🟢 Approval recommended

The changes are a mechanical, low-risk refactor with appropriate header updates and no observed behavioral issues in the modified call sites.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@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 don't see a clear advantage or necessity in switching to std::to_string here. It's just a stylistic swap. I'd suggest not making this change.

@darion-yaphet

Copy link
Copy Markdown
Contributor Author

Why should we re-implement the methods that are already provided by the standard library? There is no benefit at all.

@wasphin

wasphin commented Sep 11, 2026

Copy link
Copy Markdown
Member

Thanks for the cleanup. However, I think we need more benchmark results before making this change.

Replacing an existing implementation with std::to_string simply because the standard library provides it is not, by itself, a sufficient reason to change. We should demonstrate that the replacement is at least comparable in performance and does not introduce regressions on the toolchains we support.

In particular, the implementation of std::to_string varies across libstdc++ versions. Modern versions have optimized integer conversion, while older versions may implement it through vsnprintf, which can be significantly slower than the current butil implementation.

Could you please provide benchmarks covering representative integer types/values and, ideally, both older and newer supported GCC/libstdc++ versions? This would give us enough evidence to evaluate the trade-off rather than making the switch solely for API modernization.

@darion-yaphet

Copy link
Copy Markdown
Contributor Author

I have already explained that this PR is not intended to enhance performance, but rather to avoid duplication.

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.

4 participants