Skip to content

chore: modernize header-only code to C++17 and remove dead code#4104

Merged
ianna merged 3 commits into
mainfrom
henryiii/header-only-cpp17
Jun 22, 2026
Merged

chore: modernize header-only code to C++17 and remove dead code#4104
ianna merged 3 commits into
mainfrom
henryiii/header-only-cpp17

Conversation

@henryiii

@henryiii henryiii commented Jun 10, 2026

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

This is a cleanup PR for the standalone header-only/ C++ library (which already requires C++17). It is public API for downstream C++ users — no public signatures or behaviors are changed. Only files under header-only/ are touched.

Changes

header-only/layout-builder/awkward/utils.h — drop the hand-rolled is_integral_v/is_signed_v/is_same_v/void_t "until we switch to C++17" shims and use std::is_*_v / std::void_t.

header-only/layout-builder/awkward/LayoutBuilder.h

  • Replace the ~350 lines of BufferNBytesFunctor/ToBuffersFunctor/ToBufferFunctor/ToCharBuffersFunctor/SetIdFunctor/ClearFunctor helper classes — triplicated nearly verbatim across Record, Tuple, and Union "for C++11 compatibility" — with generic lambdas passed to visit_at. ContentsFormFunctor is intentionally left untouched to avoid conflicting with an in-flight form-bug-fix PR.
  • Remove dead state that is written but never read: Union::last_valid_index_, IndexedOption::last_valid_ (its only real use, in extend_valid, is now a local), Tuple::field_index_, and the unused/uninitialized Empty::id_. Also drop the unused index_sequence locals in Union::to_buffers/to_buffer/to_char_buffers.
  • The BitMasked class region is intentionally left alone.

header-only/growable-buffer/awkward/GrowableBuffer.h

  • Drop the const qualifier on the options_ member so that GrowableBuffer (and every LayoutBuilder that contains one) is move-assignable again, and explicitly = default the move-assignment operator (the existing user-declared move constructor otherwise suppresses it). The options() accessor stays const.
  • Use std::make_unique instead of unique_ptr(new ...) and drop a redundant std::move of a temporary.
  • Fix append() so panel growth compounds geometrically: new panels are sized from the current reserved capacity (ceil(reserved * resize)) instead of always ceil(initial * resize) — previously, with default options, every panel was the same initial-sized 1024 elements, producing N/1024 linked panels. extend() now copies each contiguous segment with a memcpy when the element type is trivially copyable (the PRIMITIVE here always is) instead of element-by-element. Lengths and contents are identical.

header-only/tests/test_1542-growable-buffer.cpp — add two focused tests for the behavior-affecting changes: multi-panel geometric growth via append(), and move assignment (which previously failed to compile because of the const member). extend() correctness across panel boundaries remains covered by the existing test_extend.

Testing

cmake -S header-only -B build -DBUILD_TESTS=ON && cmake --build build builds cleanly. The growable-buffer, builder-options, and layout-builder-is-valid test executables pass, including the new tests.

Note: test_1494-layout-builder has a pre-existing BitMasked::clear() heap-use-after-free (reproducible on main before this PR) that is being fixed separately; it is unrelated to these changes and lives in the untouched BitMasked region.

AI assistance

This PR was prepared with Claude Code as part of an automated review of the header-only library. Changes were verified by building and running the test suite.

Merge order

Two other in-flight PRs touch GrowableBuffer.h (panel-copy/concatenate-offset bug fix) and the BitMasked class, so textual conflicts are expected at merge time. This PR should be rebased and merged after henryiii/fix-bitmasked-growablebuffer and henryiii/fix-layoutbuilder-form.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.42%. Comparing base (c05b885) to head (dfcf4dd).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

henryiii added 2 commits June 11, 2026 15:41
The header-only library already requires C++17 (CMake targets declare
cxx_std_17), so this removes the pre-C++17 compatibility shims and
triplicated boilerplate, and fixes a couple of correctness/perf issues.

- utils.h: drop the hand-rolled `is_integral_v`/`is_signed_v`/`is_same_v`/
  `void_t` shims in favor of the standard `std::is_*_v` / `std::void_t`.
- LayoutBuilder.h: replace the ~350 lines of `BufferNBytesFunctor`/
  `ToBuffersFunctor`/`ToBufferFunctor`/`ToCharBuffersFunctor`/`SetIdFunctor`/
  `ClearFunctor` classes (triplicated across Record, Tuple, and Union "for
  C++11 compatibility") with generic lambdas passed to `visit_at`.
  `ContentsFormFunctor` is intentionally left untouched.
- LayoutBuilder.h: remove dead state that is written but never read:
  `Union::last_valid_index_`, `IndexedOption::last_valid_` (inlined as a
  local in `extend_valid`), `Tuple::field_index_`, and the unused
  uninitialized `Empty::id_`; also drop the unused `index_sequence` locals
  in `Union::to_buffers`/`to_buffer`/`to_char_buffers`.
- GrowableBuffer.h: drop the `const` qualifier on the `options_` member so
  that GrowableBuffer (and every LayoutBuilder containing one) is move-
  assignable again, and explicitly `= default` the move-assignment operator
  (the user-declared move constructor otherwise suppresses it). Use
  `std::make_unique` and drop a redundant `std::move` of a temporary.
- GrowableBuffer.h: fix `append()` so panel growth compounds geometrically
  (size new panels from the current reserved capacity, not from
  `initial`), and make `extend()` copy each contiguous segment with a
  `memcpy` when the type is trivially copyable. Behavior (lengths,
  contents) is unchanged; new tests cover multi-panel growth, `extend()`
  across panel boundaries, and move assignment.

The BitMasked class region is intentionally avoided to minimize conflicts
with an in-flight bug-fix PR.

Assisted-by: ClaudeCode:claude-fable-5
Remove the redundant extend-across-panels test (extend correctness is
already covered by the existing test_extend, and geometric growth by
test_append_multi_panel_growth) and drop a comment that merely restated
the fill_panel_from implementation.

Assisted-by: ClaudeCode:claude-opus-4-8
@henryiii
henryiii force-pushed the henryiii/header-only-cpp17 branch from d24e948 to b4d77db Compare June 11, 2026 19:45
@TaiSakuma TaiSakuma added the type/chore PR title type: chore (set automatically) label Jun 12, 2026

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@henryiii - Thanks! This looks great.

@ianna
ianna enabled auto-merge (squash) June 22, 2026 20:25
@ianna
ianna merged commit 3039b66 into main Jun 22, 2026
40 checks passed
@ianna
ianna deleted the henryiii/header-only-cpp17 branch June 22, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/chore PR title type: chore (set automatically)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants