Skip to content

fix: Remove explicit from std/boost hash specialisation default constructors - #8100

Merged
bthomee merged 1 commit into
XRPLF:developfrom
a1q123456:a1q123456/make-std-hash-specialisations-non-explicit
Aug 25, 2026
Merged

fix: Remove explicit from std/boost hash specialisation default constructors#8100
bthomee merged 1 commit into
XRPLF:developfrom
a1q123456:a1q123456/make-std-hash-specialisations-non-explicit

Conversation

@a1q123456

@a1q123456 a1q123456 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Remove the explicit keyword from the defaulted default constructor of five std::hash<T> / boost::hash<T> specialisations in libxrpl:

  • std::hash<xrpl::MPTID> (include/xrpl/protocol/MPTIssue.h)
  • std::hash<xrpl::MPTIssue>, std::hash<xrpl::Asset> (include/xrpl/protocol/Book.h)
  • boost::hash<xrpl::MPTIssue>, boost::hash<xrpl::Asset> (include/xrpl/protocol/Book.h)
  • boost::hash<::beast::ip::Address> (include/xrpl/beast/net/IPAddress.h)

No behavioural change; this aligns them with the convention already used by every other hash specialisation in the codebase (std::hash<xrpl::uint256>, std::hash<xrpl::Currency>, std::hash<xrpl::NodeID>, std::hash<xrpl::Directory>, std::hash<xrpl::AccountID>, and the two std::hash<xrpl::Issue> / std::hash<xrpl::Book> variants).

Context of Change

Both std::hash<T> and boost::hash<T> are required by their respective specifications to satisfy the Hash requirements, which include Cpp17DefaultConstructible. That requirement mandates the type be default-constructible in copy-initialisation contexts (i.e. T u{}; and T v = {}; must be well-formed), not just direct-initialisation.

Marking the defaulted default constructor explicit violates this: it makes copy/value-initialisation ill-formed while direct-init (T{} as a prvalue expression, T() as an operand) still compiles. This is why the bug was latent -- most direct call sites std::hash<T>{}(x) are direct-init and work either way. It only bites when a hasher is value-initialised inside a standard/boost library internal that lives in a copy-initialisation context, for example:

  • libstdc++'s _Hashtable_ebo_helper<_Hash>::_M_hash{} inside a defaulted constructor of _Hash_code_base.
  • std::tuple's primary constructor evaluating __is_implicitly_default_constructible_v on every element, which recursively probes each member via requires (void(&f)(T)) { f({}); } (copy-list-initialisation).
  • boost::unordered_* internals that value-initialise the hasher the same way libstdc++ does.

GCC 15 additionally tightened its diagnostics for [[no_unique_address]] EBO members with explicit defaulted constructors, which is what surfaces this at build time.

The cleanup is exhaustive: after this change, no hash<...> specialisation anywhere under include/ or src/ has an explicit default constructor.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping old tests)
  • Release

API Impact

  • Public API: Breaking Change
  • New feature / API
  • LibXRPL Change (any change that affects libxrpl API)
  • Peer Protocol

Test Plan

Existing compile-time and runtime coverage is sufficient: every use of the affected specialisations goes through the same hash containers already exercised by existing unit tests. No behavioural change is possible from removing explicit on a = defaulted default constructor -- the constructor body is unchanged. The fix is verified simply by the codebase compiling on GCC 15, where the previous form failed.

…structors

std::hash<T> (and boost::hash<T>) specialisations must be default-constructible in
copy-initialisation contexts because the standard library relies on this in
several places (e.g. libstdc++'s _Hashtable_ebo_helper value-initialises the
hasher via _M_hash{}, and std::tuple's primary constructor evaluates
__is_implicitly_default_constructible_v on every element -- both are
copy-initialisation contexts).

An 'explicit' default constructor makes these ill-formed. This was previously
latent because no unordered container was keyed on the affected types in a
context that triggered the instantiation chain; GCC 15 also tightened its
diagnostics for [[no_unique_address]] EBO members with explicit defaulted
constructors.

Aligns these specialisations with the existing convention already used by
std::hash<xrpl::uint256>, std::hash<xrpl::Currency>, std::hash<xrpl::NodeID>,
std::hash<xrpl::Directory>, and std::hash<xrpl::AccountID>.
@a1q123456
a1q123456 marked this pull request as ready for review August 24, 2026 16:48
@a1q123456
a1q123456 removed the request for review from kuznetsss August 24, 2026 16:49
@a1q123456 a1q123456 added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Aug 24, 2026
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

👍

@a1q123456 a1q123456 added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Aug 25, 2026
@bthomee
bthomee enabled auto-merge August 25, 2026 13:32
@bthomee
bthomee added this pull request to the merge queue Aug 25, 2026
Merged via the queue into XRPLF:develop with commit c5dc408 Aug 25, 2026
121 of 139 checks passed
@bthomee bthomee added this to the 3.4.0 milestone Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. Trivial Simple change with minimal effect, or already tested. Only needs one approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants