Adopt fcpp::vector in the public Database API#39
Open
jkalias wants to merge 4 commits into
Open
Conversation
FetchAll, Fetch(predicate) and the private FetchRecords hydration loop now return fcpp::vector<T> instead of std::vector<T>, so callers can chain functional operations (filter/map/all_of/...) directly on a query result. The hydration loop builds the result with insert_back(std::move(model)), keeping the no-extra-copy fetch path. The batch Save/SaveAutoIncrement/Update methods now take fcpp::vector<T>, with thin std::vector overloads kept for source compatibility: they copy into an fcpp::vector and forward, and the SaveAutoIncrement wrapper writes the generated ids back into the caller's std::vector. Internal plumbing (bindings, query classes, reflection metadata) stays on std::vector; only the public template API changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
Covers the point of the migration: chaining filter + map and all_of/any_of directly on a fetch result, range-for/indexing parity with std::vector, and that batch Save/Update/SaveAutoIncrement accept an fcpp::vector with ids written back. Also verifies the std::vector compatibility overloads still work, including id write-back through the SaveAutoIncrement wrapper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
Adds a "Working with query results" section showing a filter/map chain, notes that batch writes accept an fcpp::vector, and adds a migration note: auto call sites are unaffected, explicit std::vector bindings need iterator construction, and indexing is bounds-checked (aborts instead of UB). Updates the one example that bound a fetch result to std::vector, and lists functional_cpp as a dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c288909a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The std::vector compatibility wrapper copied into an fcpp::vector, ran the whole batch, then copied ids back at the end. Because each insert commits independently, a later row throwing would skip the end copy-back and leave the caller's std::vector without ids for rows that already committed. Save each record through the single-record overload instead, writing every id back as soon as its row is inserted - restoring the pre-fcpp::vector partial-success behavior and dropping the container copy. Addresses a Codex review comment on PR #39. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Extends the functional_cpp integration (which already provides
fcpp::optional_tfor nullablefields) so that query results and batch operations use
fcpp::vector<T>instead ofstd::vector<T>. Callers can now chain functional operations directly on a fetched result — e.g.db->FetchAll<Person>().filter(...).map<std::wstring>(...)— without wrapping it themselves.Scope
Only the public
Databasetemplate API changes:FetchAll<T>()andFetch<T>(const QueryPredicateBase*)now returnfcpp::vector<T>.Save/SaveAutoIncrement/Updatenow takefcpp::vector<T>, with thinstd::vectorforwarding overloads kept so existing call sites keep compiling. The overloads copy into an
fcpp::vectorand forward; theSaveAutoIncrementwrapper writes the assigned ids back into thecaller's
std::vector.FetchRecordshydration loop builds the result withinsert_back(std::move(model)),preserving the no-extra-copy fetch path.
Internal plumbing (
Bindings,GetValues, query/predicate classes,Reflection::member_metadata,StringUtilities::Join) stays onstd::vector— it's implementation detail and gains nothing fromthe functional API.
Source-breaking change (documented)
The changed return types break callers that bind a result to
std::vector<T>explicitly, sincethere is no implicit conversion back.
autocall sites, range-for, indexing, andsize()areunaffected. The README's "Working with query results" section documents the migration, including
that
fcpp::vector::operator[]is bounds-checked and callsstd::abort()on out-of-range accessinstead of being UB.
Tests
auto/indexing/size()), confirming the containerswap is behavior-preserving.
tests/functional_vector_test.ccproves the migration's point: chainingfilter+mapandall_of/any_ofon a fetch result; range-for/indexing parity; batchSave/Update/SaveAutoIncrementaccepting anfcpp::vectorwith id write-back; and thestd::vectorcompatibility overloads (including id write-back through the
SaveAutoIncrementwrapper).Verification
Full suite green in C++11, C++17, and C++20 (91 tests each: 84 existing + 7 new).
Notes
*_parallelmethods or anything behindPARALLEL_ALGORITHM_AVAILABLEare used, so the code isstandard-agnostic.
fcpp::vectoris not used in struct field declarations.