Skip to content

Preserve clamped maximum load factors - #58

Open
fhgffy wants to merge 1 commit into
Tessil:masterfrom
fhgffy:fix/preserve-clamped-load-factor
Open

fhgffy wants to merge 1 commit into
Tessil:masterfrom
fhgffy:fix/preserve-clamped-load-factor

Conversation

@fhgffy

@fhgffy fhgffy commented Oct 5, 2026

Copy link
Copy Markdown

Problem

ordered_hash::max_load_factor(float) calculates a clamped value in [0.1, 0.95], then immediately overwrites it with the original argument. This assignment was left behind when the earlier if/else clamping was converted to clamp in 899bd13.

As a result, an ordinary ordered_set<int>{3, 1, 2} with a requested load factor of 0.05f or 0.99f writes that unclamped value into its serialized header. Deserializing the library's own output then throws Invalid max_load_factor, both with and without hash compatibility. The same shared setter is used by ordered_map.

Change

Remove the redundant assignment so both the stored factor and the growth threshold use the existing clamped value. There is no change to the serialization format or the clamp implementation. Previously saved data containing an out-of-range factor remains rejected; this fix does not migrate those files.

Add regression coverage across the existing map/set test types, including vector/deque storage, strings, collision-heavy hashes and move-only values. The tests check lower/upper bounds and an in-range value, empty/populated containers, serialized header fields and both deserialization modes.

Validation

On the unmodified production header, all 88 existing test cases passed and all 11 new regression cases failed. The standalone roundtrip reproduction failed for both requested factors in both deserialization modes.

With the fix, all 99 test cases pass on Linux x86_64 with GCC 14.2:

  • C++11 debug
  • C++17 optimized (-O2)
  • C++11 optimized with -fno-exceptions
  • C++11 with AddressSanitizer and UndefinedBehaviorSanitizer

The same four upstream test translation units were compiled directly with the repository warning/assertion flags and an existing Boost.Test 1.93 shared library because a static Boost.Test library was unavailable for the repository's CMake configuration. The Boost.Test library itself was not sanitizer-instrumented. LeakSanitizer was disabled due to the environment's ptrace limitation. Clang, MSVC, macOS, 32-bit and C++ module builds were not run.

@fhgffy
fhgffy marked this pull request as ready for review October 5, 2026 00:18

This branch has not been deployed

No deployments
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.

1 participant