Skip to content

Address leftover comments from #3091#3152

Open
IvoDD wants to merge 1 commit into
masterfrom
binary-search-utils-leftover-comments
Open

Address leftover comments from #3091#3152
IvoDD wants to merge 1 commit into
masterfrom
binary-search-utils-leftover-comments

Conversation

@IvoDD

@IvoDD IvoDD commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Reference Issues/PRs

Addresses leftover comments from #3091

What does this implement or fix?

Adds testing improvements and deduplication changes. Doesn't change any logic.

Also adds a couple of makefile commands to test-cpp-repidcheck and to bench-cpp-build

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@IvoDD IvoDD added patch Small change, should increase patch version no-release-notes This PR shouldn't be added to release notes. labels Jun 4, 2026
@IvoDD IvoDD marked this pull request as ready for review June 5, 2026 06:07
@claude

claude Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

ArcticDB Code Review Summary

Test/build-only refactor addressing leftover comments from #3091 — extracts the column-builder helpers (make_single_block_column, make_regular_blocks_column, make_irregular_blocks_column, block-size generators) into the shared util/test/test_utils.hpp, deduplicating copies across test_column.cpp, rapidcheck_column.cpp, and benchmark_column.cpp; converts bound_search/gallop_bracket static_asserts into requires clauses with equivalent constraints; splits IteratorSkipsEmptyBlocks into two focused tests; and adds test-cpp-rapidcheck(-debug) / bench-cpp-build Makefile targets.

Verified: helper templates are correctly inline/template (no ODR issues), all consumers include test_utils.hpp, the new Makefile targets map to existing CMake targets (arcticdb_rapidcheck_tests, benchmarks), constraint clauses match the removed asserts, and the no-release-notes label is correctly applied. No correctness, memory, or compatibility concerns.

Minor: cpp/arcticdb/util/test/test_utils.hpp no longer ends with a trailing newline. This may fail make lint-check in CI — add a final newline.

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

Labels

no-release-notes This PR shouldn't be added to release notes. patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants