Conversation
Core SDK with context management, variant assignment, audience matching, JSON expression evaluation, and comprehensive test suite.
WalkthroughAdds a complete C++ SDK for A/B Smartly: a CMake build (C++17) producing an Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (17)
tests/CMakeLists.txt (1)
1-4: AddURL_HASHto FetchContent_Declare for integrity verification.Adding a SHA-256 hash ensures reproducible fetches and guards against tampering. The hash value for the Catch2 v3.5.2 tarball should be computed and added:
🔒 Suggested change
FetchContent_Declare( Catch2 URL https://github.com/catchorg/Catch2/archive/refs/tags/v3.5.2.tar.gz + URL_HASH SHA256=<compute via: curl -sL "https://github.com/catchorg/Catch2/archive/refs/tags/v3.5.2.tar.gz" | sha256sum> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CMakeLists.txt` around lines 1 - 4, The FetchContent_Declare block for Catch2 (the call to FetchContent_Declare with the Catch2 target and its URL) is missing an integrity hash; compute the SHA-256 of the v3.5.2 tarball and add a URL_HASH entry to that FetchContent_Declare (use the CMake form URL_HASH SHA256=<computed-hash>) so the Catch2 declaration includes the integrity check.CMakeLists.txt (1)
9-12: Add an integrity hash for the fetched nlohmann_json archive.Using
URL_HASHhelps prevent supply‑chain drift if the tarball changes. Compute the SHA‑256 for the release artefact and pin it here.🔐 Suggested pinning
FetchContent_Declare( json URL https://github.com/nlohmann/json/releases/download/v3.11.3/json.tar.xz + URL_HASH SHA256=<sha256> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 9 - 12, The FetchContent_Declare block for the dependency named "json" currently fetches the release tarball without integrity verification; compute the SHA-256 of the v3.11.3 json.tar.xz release artifact and add a URL_HASH entry to the FetchContent_Declare for the "json" target (i.e., the FetchContent_Declare(...) that references json and URL) using the format expected by CMake so the fetch is pinned to that SHA-256 digest.src/utils.cpp (1)
55-64: Make empty split handling explicit.Line 63 currently relies on
split.size() - 1, which underflows to ‑1 when the split is empty. An explicit guard clarifies the contract and avoids accidental indexing by callers.♻️ Suggested guard
int choose_variant(const std::vector<double>& split, double probability) { + if (split.empty()) { + return -1; + } double cum_sum = 0.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.cpp` around lines 55 - 64, The function choose_variant currently underflows when split is empty; add an explicit guard at the start of choose_variant that checks split.empty() and fails fast (e.g., throw std::invalid_argument("choose_variant: split must not be empty")) so callers can't accidentally rely on split.size()-1; update error message to include the function name and ensure the header <stdexcept> is included where appropriate.src/context.cpp (5)
14-20:array_equals_shallowis equivalent tostd::vector::operator==.
std::vector<double>::operator==already performs element-wise comparison. This helper can be replaced bya == b.♻️ Proposed simplification
-static bool array_equals_shallow(const std::vector<double>& a, const std::vector<double>& b) { - if (a.size() != b.size()) return false; - for (size_t i = 0; i < a.size(); ++i) { - if (a[i] != b[i]) return false; - } - return true; -}Then at the call site (line 654):
- && array_equals_shallow(experiment.trafficSplit, assignment.traffic_split); + && experiment.trafficSplit == assignment.traffic_split;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 14 - 20, The helper function array_equals_shallow duplicates std::vector<double>::operator==; remove the array_equals_shallow definition and replace every call site (e.g., occurrences where array_equals_shallow(a,b) is used) with the direct comparison (a == b) using the original vector variables; ensure headers remain for std::vector and that no other code expects a non-boolean behavior from array_equals_shallow before deleting it.
40-41: Redundantdata_re-assignment ininit().The constructor already initialises
data_via the member initialiser list (line 25:data_(std::move(data))), then callsinit(data_)which re-assignsdata_ = data(line 575). This copiesdata_back onto itself — harmless but wasteful. Consider havinginitonly callbuild_index(), or removing the redundant assignment.♻️ Proposed fix
void Context::init(const ContextData& data) { - data_ = data; build_index(); }Or inline the call in the constructor:
- init(data_); + build_index();Also applies to: 574-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 40 - 41, The constructor sets data_ via the member initializer list then immediately calls init(data_) which reassigns data_ to itself; remove the redundant assignment by changing init to not reassign data_ (i.e., drop `data_ = data` inside init) and have init only call build_index(), or alternatively change the constructor to call build_index() directly instead of init(data_); update references to init() and the constructor accordingly (symbols: init, build_index, data_).
309-337:track()only stores numeric properties inGoalAchievementbut emits all properties in the event.Lines 318–321 filter
goal.propertiesto numeric values only, but line 330 emits the originalpropertiesJSON (including non-numeric values) in the event payload. If this is intentional (event handler sees everything, but the publish payload is filtered), it's worth a brief comment to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 309 - 337, Context::track currently stores only numeric entries into GoalAchievement::properties but emits the original properties JSON to emit_event, causing a mismatch; change the emission to reflect the filtered stored properties by converting goal.properties into a nlohmann::json (or null when empty) and assign that to goal_data["properties"] before calling emit_event("goal", goal_data) so the event payload matches the stored GoalAchievement, referencing Context::track, GoalAchievement::properties, and emit_event.
412-540:get_or_create_assignment— correct but dense; consider extracting sub-functions.This 130-line method handles cache invalidation, overrides, audience matching, traffic eligibility, custom assignments, full-on variants, and variable resolution. Breaking it into smaller helpers (e.g.,
compute_assignment_for_experiment,apply_override) would improve readability and testability without changing behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 412 - 540, The get_or_create_assignment function is too large and should be split into smaller helpers to improve readability and testability; refactor by extracting the distinct responsibilities into methods such as apply_override(Assignment&, const ExperimentIndex*, const std::string& experiment_name), evaluate_audience(const ExperimentData&, Assignment&), determine_variant_and_eligibility(const ExperimentData&, Assignment&, const std::string& unit_type), and resolve_variables_if_applicable(Assignment&, const ExperimentIndex*), then rewrite get_or_create_assignment to perform high-level flow: check cache/overrides, call apply_override when overrides_ exists, otherwise call evaluate_audience then determine_variant_and_eligibility, set meta fields (id, iteration, traffic_split, attrs_seq), call resolve_variables_if_applicable, and finally store/move the Assignment into assignments_ so behavior remains identical but logic is modularized (use existing symbols: get_or_create_assignment, assignments_, overrides_, index_, audience_matcher_, assigners_, units_, cassignments_).
616-646: Manual sorted insertion inbuild_indexcould be simplified.The nested loop with manual insertion-point search for
index_variables_can be replaced withstd::lower_boundfor clarity and to avoid the duplicate-check logic interleaved with insertion.♻️ Simplified with std::lower_bound
- if (!found) { - bool inserted = false; - for (auto it = vec.begin(); it != vec.end(); ++it) { - if (entry.data->id < (*it)->data->id) { - vec.insert(it, &entry); - inserted = true; - break; - } - if (entry.data->id == (*it)->data->id) { - inserted = true; - break; - } - } - if (!inserted) { - vec.push_back(&entry); - } - } + if (!found) { + auto pos = std::lower_bound( + vec.begin(), vec.end(), &entry, + [](const ExperimentIndex* a, const ExperimentIndex* b) { + return a->data->id < b->data->id; + }); + if (pos == vec.end() || (*pos)->data->id != entry.data->id) { + vec.insert(pos, &entry); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 616 - 646, The manual sorted insertion in build_index that walks vec to insert &entry (using index_variables_, index_, and entry) should be replaced with std::lower_bound to find the insertion position and a single duplicate check; for each key get auto& vec = index_variables_[key], use std::lower_bound(vec.begin(), vec.end(), &entry, [](const ContextEntry* a, const ContextEntry* b){ return a->data->id < b->data->id; }) to obtain the iterator, then if the iterator is at end or (*it)->data->id != entry.data->id (or pointer inequality) call vec.insert(it, &entry); this removes the manual loop/flags and requires adding `#include` <algorithm> and using the existing symbols index_variables_, entry, index_, and build_index.src/json_expr/evaluator.cpp (3)
4-4: Unused include<charconv>.This header is included but never used — the implementation uses
std::stodandstd::ostringstreaminstead ofstd::from_chars/std::to_chars.🧹 Remove unused include
`#include` <cmath> -#include <charconv> `#include` <sstream>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/json_expr/evaluator.cpp` at line 4, Remove the unused `#include` <charconv> from src/json_expr/evaluator.cpp: the implementation uses std::stod and std::ostringstream (see functions around evaluator.cpp where std::stod and std::ostringstream are used), so delete the <charconv> include to avoid an unnecessary dependency and compilation warning.
35-43: Object expression dispatch relies on JSON key iteration order.The loop iterates the object's keys, checks only the first key against the operator map, and immediately
breaks. This is intentional for the single-key expression format, butnlohmann::jsonobjects iterate in alphabetical order (they usestd::mapinternally by default). This means{"gt": [...], "value": 5}would match"gt"rather than"value", which is fine — but it's worth a comment to make the assumption explicit for maintainers.📝 Suggested inline comment
if (expr.is_object()) { + // Expression objects must contain exactly one operator key. + // We check only the first key (alphabetical order in nlohmann::json). for (auto it = expr.begin(); it != expr.end(); ++it) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/json_expr/evaluator.cpp` around lines 35 - 43, The object-dispatch in Evaluator::evaluate relies on iterating a nlohmann::json object and using only the first key to find an operator in operators_, which depends on nlohmann::json's alphabetical iteration order and the convention that expressions are single-key objects; update the evaluate function to include an explicit inline comment documenting that assumption (mention nlohmann::json uses std::map and iterates keys alphabetically), and optionally add a small runtime check (e.g., assert or throw) that the object has exactly one key before using the first key to select the operator to make the contract explicit; reference the evaluate method and the operators_ lookup in your change.
139-148: Direct floating-point equality comparison incompare.
lv == rvat line 144 uses directdoubleequality. For the intended use case (small integers and simple decimals in A/B testing config), this is acceptable. Just flagging that an epsilon-based comparison would be more robust if fractional arithmetic ever feeds into comparison operands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/json_expr/evaluator.cpp` around lines 139 - 148, The compare logic in function compare uses direct double equality (lv == rv) which is fragile; change it to an epsilon-based comparison: compute if (std::fabs(lv - rv) < EPS) treat as equal, else return lv > rv ? 1 : -1; define a small constant EPS (e.g. 1e-9) scoped to the file or function and include <cmath> or use std::abs, and update the branch in evaluator.cpp where lv and rv are compared to use this epsilon comparison.src/json_expr/operators.cpp (2)
243-275:std::regexis compiled on every evaluation — acceptable but worth noting.
std::regexconstruction is expensive. IfMatchOperatoris evaluated repeatedly with the same pattern (e.g., during bulk audience matching), this could become a hot path. Not critical for typical A/B testing workloads, but a regex cache could be added later if profiling shows contention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/json_expr/operators.cpp` around lines 243 - 275, MatchOperator::evaluate currently constructs std::regex every call which is expensive; add a simple cache of compiled regex objects keyed by the pattern string in the MatchOperator class (e.g., an std::unordered_map<std::string, std::regex> member) and lookup/insert before constructing a new std::regex inside MatchOperator::evaluate; protect the map with a mutex or shared_mutex for thread-safety when reading/writing and reuse the cached std::regex for std::regex_search, falling back to compiling and caching on regex construction errors (same error handling as today).
104-194: Significant code duplication acrossGt,Gte,Lt,Lteoperators.These four operators share identical boilerplate (array check, lhs/rhs evaluation, null guards, compare call) and differ only in the comparison predicate. Extracting a shared helper would reduce ~90 lines to ~20 and make adding new comparison operators trivial.
♻️ Proposed helper-based refactor
+// In a shared location (e.g., anonymous namespace in operators.cpp): +static nlohmann::json compare_op( + const Evaluator& evaluator, + const nlohmann::json& args, + const nlohmann::json& vars, + std::function<bool(int)> predicate +) { + if (!args.is_array()) return nullptr; + auto lhs = args.size() > 0 ? evaluator.evaluate(args[0], vars) : nlohmann::json(nullptr); + if (lhs.is_null()) return nullptr; + auto rhs = args.size() > 1 ? evaluator.evaluate(args[1], vars) : nlohmann::json(nullptr); + if (rhs.is_null()) return nullptr; + auto result = Evaluator::compare(lhs, rhs); + if (result.has_value()) return predicate(*result); + return nullptr; +} + nlohmann::json GtOperator::evaluate( const Evaluator& evaluator, const nlohmann::json& args, const nlohmann::json& vars ) const { - if (!args.is_array()) { - return nullptr; - } - auto lhs = args.size() > 0 ? evaluator.evaluate(args[0], vars) : nlohmann::json(nullptr); - if (lhs.is_null()) { - return nullptr; - } - auto rhs = args.size() > 1 ? evaluator.evaluate(args[1], vars) : nlohmann::json(nullptr); - if (rhs.is_null()) { - return nullptr; - } - auto result = Evaluator::compare(lhs, rhs); - if (result.has_value()) { - return *result > 0; - } - return nullptr; + return compare_op(evaluator, args, vars, [](int r) { return r > 0; }); }Apply similarly to
GteOperator,LtOperator, andLteOperator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/json_expr/operators.cpp` around lines 104 - 194, The four evaluate methods (GtOperator::evaluate, GteOperator::evaluate, LtOperator::evaluate, LteOperator::evaluate) duplicate the same argument checking, lhs/rhs evaluation, null guards and call to Evaluator::compare; extract a shared helper (e.g., evaluateComparison or evaluateBinaryComparison) that takes (const Evaluator& evaluator, const nlohmann::json& args, const nlohmann::json& vars, std::function<bool(int)> predicate) and encapsulates: array check, evaluate lhs/rhs, null checks, call Evaluator::compare and return predicate(result) as nlohmann::json or nullptr on failure; then rewrite each operator (GtOperator::evaluate, GteOperator::evaluate, LtOperator::evaluate, LteOperator::evaluate) to call this helper with the appropriate lambda (r>0, r>=0, r<0, r<=0).include/absmartly/context.h (2)
16-16:<chrono>is unused in this header.
std::chronois used only incontext.cpp(vianow_millis). Moving it to the implementation file reduces transitive include costs.🧹 Move to implementation file
-#include <chrono>And ensure
#include <chrono>is present insrc/context.cpp(it already is at line 5).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/context.h` at line 16, Remove the unused header include <chrono> from include/absmartly/context.h because std::chrono is only used in the implementation (now_millis in context.cpp); ensure `#include` <chrono> remains (or is added) in src/context.cpp so the implementation compiles, and run a quick build to confirm no transitive include breakage.
20-42:AssignmentandExperimentIndexare implementation details leaked into the public header.These structs are used internally by
Context(private members) and are not part of the public API surface. Exposing them in the header couples consumers to internal representation. Consider forward-declaring them and defining them in an internal header or incontext.cpp.This is fine for an initial implementation and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/context.h` around lines 20 - 42, The public header is leaking internal implementation details by defining struct Assignment and struct ExperimentIndex; instead, forward-declare these types in the public header and move their full definitions into an internal header or context.cpp. Update Context's private members to use opaque pointers (e.g., std::unique_ptr<Assignment>/ExperimentIndex or raw pointers) so the compiler only needs the forward declarations in include/absmartly/context.h, and add the moved definitions and any required includes (nlohmann::json, vector, map, ExperimentData) to the internal header or context.cpp where Context's methods are implemented.include/absmartly/json_expr/operators.h (1)
3-6: Remove<regex>from the header — it is only used in the.cppimplementation.
<regex>is a notoriously heavy header and is only needed byMatchOperator::evaluateinoperators.cpp. Including it here inflates compile times for every translation unit that includesoperators.h.🧹 Proposed fix
`#include` <memory> `#include` <optional> -#include <regex> `#include` <string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/json_expr/operators.h` around lines 3 - 6, The header operators.h unnecessarily includes <regex>, which increases compile times; remove the `#include` <regex> from operators.h and ensure the implementation file (operators.cpp) includes <regex> so MatchOperator::evaluate has the needed declaration; keep other headers (<memory>, <optional>, <string>) in operators.h and verify no other declarations in the header depend on std::regex.tests/json_expr_test.cpp (1)
332-416: Good integration tests — consider adding edge-case coverage fornulloperator andin/matchoperators.The integration tests effectively validate composite audience-matching expressions. You may want to add integration-level tests for the
null,in, andmatchoperators as well, since they are registered in theEvaluatorbut not exercised here (they may be tested inoperators_test.cpp, but integration-level coverage would strengthen confidence).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/json_expr_test.cpp` around lines 332 - 416, Add integration tests that exercise the registered 'null', 'in', and 'match' operators by creating new SECTIONs that call Evaluator::evaluate_boolean with appropriate json expressions built using the existing helpers (value_for, var_for, binary_op, unary_op) and the same sample subjects (john, terry, kate, maria); for 'null' construct an expression that checks a missing or explicitly null field, for 'in' use binary_op("in", var_for("language"), value_for(json::array({...}))) to assert membership, and for 'match' use binary_op("match", var_for("language"), value_for("regex-or-pattern")) to validate pattern matching — reuse the existing test structure and REQUIRE assertions similar to other SECTIONs to verify expected boolean results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/absmartly/models.h`:
- Around line 42-47: The from_json deserializer for CustomFieldValue can leave
stale data in CustomFieldValue::value when the JSON "value" key is missing or
null; update inline void from_json(const nlohmann::json& j, CustomFieldValue& v)
to explicitly reset v.value to std::nullopt in the else path (i.e., when
!j.contains("value") or j.at("value").is_null()), leaving the existing get logic
when a non-null value is present so existing instances get cleared correctly.
In `@README.md`:
- Around line 11-37: The Installation section in README.md uses H2 then jumps to
H4 (the "#### CMake (FetchContent)", "#### CMake (Local)", and "#### Building
from Source" headings); update those subsection headings to H3 (change #### to
###) so heading levels increment properly and satisfy markdownlint's
heading-increment rule—ensure all related subsection headings under the
Installation H2 are consistently H3.
- Around line 185-187: Add a comma after "logs in" in the sentence describing
when to call set_unit() or set_units() to improve readability; update the README
sentence mentioning set_unit(), set_units() and context so it reads "...when a
user logs in, and you want to associate..." (keep references to set_unit(),
set_units(), and context unchanged).
In `@src/context.cpp`:
- Around line 273-278: The code in custom_field_value uses std::stod (in the
block handling field.type == "number"), which is locale-dependent; replace the
std::stod call with a locale-independent parser such as std::from_chars (include
<charconv>), parse val into a double using std::from_chars, check the returned
error code (std::errc or std::from_chars_result) and return nullptr on failure
(same behavior as the current catch path), and remove the try/catch; update the
custom_field_value function to handle the from_chars result and preserve the
existing return semantics.
- Around line 153-155: set_override currently mutates overrides_ after
finalisation while set_custom_assignment enforces immutability via
check_not_finalized(); call check_not_finalized() at the start of both
Context::set_override and Context::set_overrides to prevent post-finalisation
mutation and keep behavior consistent with set_custom_assignment (or, if
overrides are intentionally mutable, add explicit documentation to that effect
instead). Ensure you reference the check_not_finalized() guard and the
overrides_ mutations in these methods so tests expecting post-finalisation
mutation to be disallowed will pass.
In `@src/json_expr/evaluator.cpp`:
- Around line 112-124: The number-to-string path in src/json_expr/evaluator.cpp
uses std::ostringstream (the two local variables named oss) which pick up the
global locale; imbue each ostringstream with the C locale (call
oss.imbue(std::locale::classic()) immediately after construction) before
formatting so decimal separators are always '.'; also ensure <locale> is
included if not already.
- Around line 84-100: The string-to-double conversion uses locale-sensitive
std::stod; replace it with locale-independent std::from_chars (include
<charconv>) to parse the std::string 'str' (or a std::string_view) into a double
named 'result', remove the try/catch and the pos variable, then check that
from_chars returned no error and consumed the entire input (the end pointer
equals str.data()+str.size()) and that std::isfinite(result) before returning
result; otherwise return std::nullopt — update the conversion logic in the block
that currently references value, str, pos, result to use std::from_chars
instead.
In `@src/md5.cpp`:
- Around line 169-174: The code currently writes only a 32-bit length into
block[14], truncating inputs ≥512MB; compute the full 64-bit bit-length (e.g.
uint64_t bitlen = static_cast<uint64_t>(len) << 3), then store the low 32 bits
into block[14] and the high 32 bits into block[15] before calling
md5cycle(state, block) so the MD5 length is encoded as a 64-bit value.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 9-12: The FetchContent_Declare block for the dependency named
"json" currently fetches the release tarball without integrity verification;
compute the SHA-256 of the v3.11.3 json.tar.xz release artifact and add a
URL_HASH entry to the FetchContent_Declare for the "json" target (i.e., the
FetchContent_Declare(...) that references json and URL) using the format
expected by CMake so the fetch is pinned to that SHA-256 digest.
In `@include/absmartly/context.h`:
- Line 16: Remove the unused header include <chrono> from
include/absmartly/context.h because std::chrono is only used in the
implementation (now_millis in context.cpp); ensure `#include` <chrono> remains (or
is added) in src/context.cpp so the implementation compiles, and run a quick
build to confirm no transitive include breakage.
- Around line 20-42: The public header is leaking internal implementation
details by defining struct Assignment and struct ExperimentIndex; instead,
forward-declare these types in the public header and move their full definitions
into an internal header or context.cpp. Update Context's private members to use
opaque pointers (e.g., std::unique_ptr<Assignment>/ExperimentIndex or raw
pointers) so the compiler only needs the forward declarations in
include/absmartly/context.h, and add the moved definitions and any required
includes (nlohmann::json, vector, map, ExperimentData) to the internal header or
context.cpp where Context's methods are implemented.
In `@include/absmartly/json_expr/operators.h`:
- Around line 3-6: The header operators.h unnecessarily includes <regex>, which
increases compile times; remove the `#include` <regex> from operators.h and ensure
the implementation file (operators.cpp) includes <regex> so
MatchOperator::evaluate has the needed declaration; keep other headers
(<memory>, <optional>, <string>) in operators.h and verify no other declarations
in the header depend on std::regex.
In `@src/context.cpp`:
- Around line 14-20: The helper function array_equals_shallow duplicates
std::vector<double>::operator==; remove the array_equals_shallow definition and
replace every call site (e.g., occurrences where array_equals_shallow(a,b) is
used) with the direct comparison (a == b) using the original vector variables;
ensure headers remain for std::vector and that no other code expects a
non-boolean behavior from array_equals_shallow before deleting it.
- Around line 40-41: The constructor sets data_ via the member initializer list
then immediately calls init(data_) which reassigns data_ to itself; remove the
redundant assignment by changing init to not reassign data_ (i.e., drop `data_ =
data` inside init) and have init only call build_index(), or alternatively
change the constructor to call build_index() directly instead of init(data_);
update references to init() and the constructor accordingly (symbols: init,
build_index, data_).
- Around line 309-337: Context::track currently stores only numeric entries into
GoalAchievement::properties but emits the original properties JSON to
emit_event, causing a mismatch; change the emission to reflect the filtered
stored properties by converting goal.properties into a nlohmann::json (or null
when empty) and assign that to goal_data["properties"] before calling
emit_event("goal", goal_data) so the event payload matches the stored
GoalAchievement, referencing Context::track, GoalAchievement::properties, and
emit_event.
- Around line 412-540: The get_or_create_assignment function is too large and
should be split into smaller helpers to improve readability and testability;
refactor by extracting the distinct responsibilities into methods such as
apply_override(Assignment&, const ExperimentIndex*, const std::string&
experiment_name), evaluate_audience(const ExperimentData&, Assignment&),
determine_variant_and_eligibility(const ExperimentData&, Assignment&, const
std::string& unit_type), and resolve_variables_if_applicable(Assignment&, const
ExperimentIndex*), then rewrite get_or_create_assignment to perform high-level
flow: check cache/overrides, call apply_override when overrides_ exists,
otherwise call evaluate_audience then determine_variant_and_eligibility, set
meta fields (id, iteration, traffic_split, attrs_seq), call
resolve_variables_if_applicable, and finally store/move the Assignment into
assignments_ so behavior remains identical but logic is modularized (use
existing symbols: get_or_create_assignment, assignments_, overrides_, index_,
audience_matcher_, assigners_, units_, cassignments_).
- Around line 616-646: The manual sorted insertion in build_index that walks vec
to insert &entry (using index_variables_, index_, and entry) should be replaced
with std::lower_bound to find the insertion position and a single duplicate
check; for each key get auto& vec = index_variables_[key], use
std::lower_bound(vec.begin(), vec.end(), &entry, [](const ContextEntry* a, const
ContextEntry* b){ return a->data->id < b->data->id; }) to obtain the iterator,
then if the iterator is at end or (*it)->data->id != entry.data->id (or pointer
inequality) call vec.insert(it, &entry); this removes the manual loop/flags and
requires adding `#include` <algorithm> and using the existing symbols
index_variables_, entry, index_, and build_index.
In `@src/json_expr/evaluator.cpp`:
- Line 4: Remove the unused `#include` <charconv> from
src/json_expr/evaluator.cpp: the implementation uses std::stod and
std::ostringstream (see functions around evaluator.cpp where std::stod and
std::ostringstream are used), so delete the <charconv> include to avoid an
unnecessary dependency and compilation warning.
- Around line 35-43: The object-dispatch in Evaluator::evaluate relies on
iterating a nlohmann::json object and using only the first key to find an
operator in operators_, which depends on nlohmann::json's alphabetical iteration
order and the convention that expressions are single-key objects; update the
evaluate function to include an explicit inline comment documenting that
assumption (mention nlohmann::json uses std::map and iterates keys
alphabetically), and optionally add a small runtime check (e.g., assert or
throw) that the object has exactly one key before using the first key to select
the operator to make the contract explicit; reference the evaluate method and
the operators_ lookup in your change.
- Around line 139-148: The compare logic in function compare uses direct double
equality (lv == rv) which is fragile; change it to an epsilon-based comparison:
compute if (std::fabs(lv - rv) < EPS) treat as equal, else return lv > rv ? 1 :
-1; define a small constant EPS (e.g. 1e-9) scoped to the file or function and
include <cmath> or use std::abs, and update the branch in evaluator.cpp where lv
and rv are compared to use this epsilon comparison.
In `@src/json_expr/operators.cpp`:
- Around line 243-275: MatchOperator::evaluate currently constructs std::regex
every call which is expensive; add a simple cache of compiled regex objects
keyed by the pattern string in the MatchOperator class (e.g., an
std::unordered_map<std::string, std::regex> member) and lookup/insert before
constructing a new std::regex inside MatchOperator::evaluate; protect the map
with a mutex or shared_mutex for thread-safety when reading/writing and reuse
the cached std::regex for std::regex_search, falling back to compiling and
caching on regex construction errors (same error handling as today).
- Around line 104-194: The four evaluate methods (GtOperator::evaluate,
GteOperator::evaluate, LtOperator::evaluate, LteOperator::evaluate) duplicate
the same argument checking, lhs/rhs evaluation, null guards and call to
Evaluator::compare; extract a shared helper (e.g., evaluateComparison or
evaluateBinaryComparison) that takes (const Evaluator& evaluator, const
nlohmann::json& args, const nlohmann::json& vars, std::function<bool(int)>
predicate) and encapsulates: array check, evaluate lhs/rhs, null checks, call
Evaluator::compare and return predicate(result) as nlohmann::json or nullptr on
failure; then rewrite each operator (GtOperator::evaluate,
GteOperator::evaluate, LtOperator::evaluate, LteOperator::evaluate) to call this
helper with the appropriate lambda (r>0, r>=0, r<0, r<=0).
In `@src/utils.cpp`:
- Around line 55-64: The function choose_variant currently underflows when split
is empty; add an explicit guard at the start of choose_variant that checks
split.empty() and fails fast (e.g., throw std::invalid_argument("choose_variant:
split must not be empty")) so callers can't accidentally rely on split.size()-1;
update error message to include the function name and ensure the header
<stdexcept> is included where appropriate.
In `@tests/CMakeLists.txt`:
- Around line 1-4: The FetchContent_Declare block for Catch2 (the call to
FetchContent_Declare with the Catch2 target and its URL) is missing an integrity
hash; compute the SHA-256 of the v3.5.2 tarball and add a URL_HASH entry to that
FetchContent_Declare (use the CMake form URL_HASH SHA256=<computed-hash>) so the
Catch2 declaration includes the integrity check.
In `@tests/json_expr_test.cpp`:
- Around line 332-416: Add integration tests that exercise the registered
'null', 'in', and 'match' operators by creating new SECTIONs that call
Evaluator::evaluate_boolean with appropriate json expressions built using the
existing helpers (value_for, var_for, binary_op, unary_op) and the same sample
subjects (john, terry, kate, maria); for 'null' construct an expression that
checks a missing or explicitly null field, for 'in' use binary_op("in",
var_for("language"), value_for(json::array({...}))) to assert membership, and
for 'match' use binary_op("match", var_for("language"),
value_for("regex-or-pattern")) to validate pattern matching — reuse the existing
test structure and REQUIRE assertions similar to other SECTIONs to verify
expected boolean results.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
CMakeLists.txtREADME.mdinclude/absmartly/audience_matcher.hinclude/absmartly/context.hinclude/absmartly/context_config.hinclude/absmartly/context_event_handler.hinclude/absmartly/errors.hinclude/absmartly/hashing.hinclude/absmartly/json_expr/evaluator.hinclude/absmartly/json_expr/operators.hinclude/absmartly/models.hinclude/absmartly/sdk.hinclude/absmartly/variant_assigner.hsrc/audience_matcher.cppsrc/context.cppsrc/json_expr/evaluator.cppsrc/json_expr/operators.cppsrc/md5.cppsrc/murmur3.cppsrc/utils.cppsrc/variant_assigner.cpptests/CMakeLists.txttests/audience_matcher_test.cpptests/context_test.cpptests/json_expr_test.cpptests/md5_test.cpptests/murmur3_test.cpptests/operators_test.cpptests/utils_test.cpptests/variant_assigner_test.cpp
| inline void from_json(const nlohmann::json& j, CustomFieldValue& v) { | ||
| j.at("name").get_to(v.name); | ||
| j.at("type").get_to(v.type); | ||
| if (j.contains("value") && !j.at("value").is_null()) { | ||
| v.value = j.at("value").get<std::string>(); | ||
| } |
There was a problem hiding this comment.
Reset CustomFieldValue::value when JSON is null or missing.
If from_json is called on an existing instance, a null/missing value leaves stale data in v.value. Resetting to std::nullopt avoids that.
🛠️ Suggested fix
inline void from_json(const nlohmann::json& j, CustomFieldValue& v) {
j.at("name").get_to(v.name);
j.at("type").get_to(v.type);
if (j.contains("value") && !j.at("value").is_null()) {
v.value = j.at("value").get<std::string>();
+ } else {
+ v.value = std::nullopt;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline void from_json(const nlohmann::json& j, CustomFieldValue& v) { | |
| j.at("name").get_to(v.name); | |
| j.at("type").get_to(v.type); | |
| if (j.contains("value") && !j.at("value").is_null()) { | |
| v.value = j.at("value").get<std::string>(); | |
| } | |
| inline void from_json(const nlohmann::json& j, CustomFieldValue& v) { | |
| j.at("name").get_to(v.name); | |
| j.at("type").get_to(v.type); | |
| if (j.contains("value") && !j.at("value").is_null()) { | |
| v.value = j.at("value").get<std::string>(); | |
| } else { | |
| v.value = std::nullopt; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/absmartly/models.h` around lines 42 - 47, The from_json deserializer
for CustomFieldValue can leave stale data in CustomFieldValue::value when the
JSON "value" key is missing or null; update inline void from_json(const
nlohmann::json& j, CustomFieldValue& v) to explicitly reset v.value to
std::nullopt in the else path (i.e., when !j.contains("value") or
j.at("value").is_null()), leaving the existing get logic when a non-null value
is present so existing instances get cleared correctly.
| #### CMake (FetchContent) | ||
|
|
||
| Add the following to your `CMakeLists.txt` to include the SDK directly from the repository: | ||
|
|
||
| ```cmake | ||
| include(FetchContent) | ||
|
|
||
| FetchContent_Declare( | ||
| absmartly-sdk | ||
| GIT_REPOSITORY https://github.com/absmartly/cpp-sdk.git | ||
| GIT_TAG main | ||
| ) | ||
| FetchContent_MakeAvailable(absmartly-sdk) | ||
|
|
||
| target_link_libraries(your_target PRIVATE absmartly-sdk) | ||
| ``` | ||
|
|
||
| #### CMake (Local) | ||
|
|
||
| If you have cloned the repository locally, you can add it as a subdirectory: | ||
|
|
||
| ```cmake | ||
| add_subdirectory(path/to/cpp-sdk) | ||
| target_link_libraries(your_target PRIVATE absmartly-sdk) | ||
| ``` | ||
|
|
||
| #### Building from Source |
There was a problem hiding this comment.
Fix heading level jumps in the Installation section.
Line 11 (and the following subsection headings) jump from H2 to H4, which triggers markdownlint’s heading increment rule. Consider using H3 for these subsections.
Proposed change
-#### CMake (FetchContent)
+### CMake (FetchContent)
@@
-#### CMake (Local)
+### CMake (Local)
@@
-#### Building from Source
+### Building from Source📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### CMake (FetchContent) | |
| Add the following to your `CMakeLists.txt` to include the SDK directly from the repository: | |
| ```cmake | |
| include(FetchContent) | |
| FetchContent_Declare( | |
| absmartly-sdk | |
| GIT_REPOSITORY https://github.com/absmartly/cpp-sdk.git | |
| GIT_TAG main | |
| ) | |
| FetchContent_MakeAvailable(absmartly-sdk) | |
| target_link_libraries(your_target PRIVATE absmartly-sdk) | |
| ``` | |
| #### CMake (Local) | |
| If you have cloned the repository locally, you can add it as a subdirectory: | |
| ```cmake | |
| add_subdirectory(path/to/cpp-sdk) | |
| target_link_libraries(your_target PRIVATE absmartly-sdk) | |
| ``` | |
| #### Building from Source | |
| ### CMake (FetchContent) | |
| Add the following to your `CMakeLists.txt` to include the SDK directly from the repository: | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 11-11: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 11 - 37, The Installation section in README.md uses
H2 then jumps to H4 (the "#### CMake (FetchContent)", "#### CMake (Local)", and
"#### Building from Source" headings); update those subsection headings to H3
(change #### to ###) so heading levels increment properly and satisfy
markdownlint's heading-increment rule—ensure all related subsection headings
under the Installation H2 are consistently H3.
| ### Setting Extra Units | ||
|
|
||
| You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in and you want to associate the new identity with the context. Note that you cannot override an already set unit type, as that would be a change of identity and will throw an exception. In this case, you must create a new context instead. |
There was a problem hiding this comment.
Minor punctuation tweak for readability.
Line 187 reads more smoothly with a comma after “logs in”.
Proposed change
-You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in and you want to associate the new identity with the context.
+You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in, and you want to associate the new identity with the context.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Setting Extra Units | |
| You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in and you want to associate the new identity with the context. Note that you cannot override an already set unit type, as that would be a change of identity and will throw an exception. In this case, you must create a new context instead. | |
| ### Setting Extra Units | |
| You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in, and you want to associate the new identity with the context. Note that you cannot override an already set unit type, as that would be a change of identity and will throw an exception. In this case, you must create a new context instead. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...hods. This is useful when a user logs in and you want to associate the new identity ...
(COMMA_COMPOUND_SENTENCE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 185 - 187, Add a comma after "logs in" in the
sentence describing when to call set_unit() or set_units() to improve
readability; update the README sentence mentioning set_unit(), set_units() and
context so it reads "...when a user logs in, and you want to associate..." (keep
references to set_unit(), set_units(), and context unchanged).
| void Context::set_override(const std::string& experiment_name, int variant) { | ||
| overrides_[experiment_name] = variant; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/context.cpp | sed -n '145,175p'Repository: absmartly/cpp-sdk
Length of output: 1248
🏁 Script executed:
rg -n "check_not_finalized" --type=cpp -B2 -A2Repository: absmartly/cpp-sdk
Length of output: 2885
🏁 Script executed:
rg -n "set_override|set_custom_assignment" --type=cpp -B3 -A5Repository: absmartly/cpp-sdk
Length of output: 11180
🏁 Script executed:
find . -name "*test*.cpp" -o -name "*test*.h" | head -20Repository: absmartly/cpp-sdk
Length of output: 276
🏁 Script executed:
rg -n "override|finalize" include/absmartly/context.h -B2 -A2Repository: absmartly/cpp-sdk
Length of output: 1187
🏁 Script executed:
cat -n src/context.cpp | sed -n '700,710p'Repository: absmartly/cpp-sdk
Length of output: 426
🏁 Script executed:
rg -n "should throw after finalize" tests/context_test.cpp -B5 -A10Repository: absmartly/cpp-sdk
Length of output: 3843
set_override allows state mutation on finalised contexts — inconsistent with set_custom_assignment.
set_custom_assignment (line 164) calls check_not_finalized() to prevent mutations after finalisation, but set_override does not. The test suite (line 837–841) explicitly verifies that set_custom_assignment throws after finalisation, yet contains no corresponding test for set_override. If overrides are intentionally permitted post-finalisation, this should be documented; otherwise, add check_not_finalized() to set_override and set_overrides for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.cpp` around lines 153 - 155, set_override currently mutates
overrides_ after finalisation while set_custom_assignment enforces immutability
via check_not_finalized(); call check_not_finalized() at the start of both
Context::set_override and Context::set_overrides to prevent post-finalisation
mutation and keep behavior consistent with set_custom_assignment (or, if
overrides are intentionally mutable, add explicit documentation to that effect
instead). Ensure you reference the check_not_finalized() guard and the
overrides_ mutations in these methods so tests expecting post-finalisation
mutation to be disallowed will pass.
| if (field.type == "number") { | ||
| try { | ||
| return std::stod(val); | ||
| } catch (...) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
std::stod in custom_field_value has the same locale sensitivity issue as in evaluator.cpp.
For "number" type custom fields, std::stod is locale-dependent. This mirrors the issue flagged in evaluator.cpp. Consider using std::from_chars or a locale-independent parsing approach here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.cpp` around lines 273 - 278, The code in custom_field_value uses
std::stod (in the block handling field.type == "number"), which is
locale-dependent; replace the std::stod call with a locale-independent parser
such as std::from_chars (include <charconv>), parse val into a double using
std::from_chars, check the returned error code (std::errc or
std::from_chars_result) and return nullptr on failure (same behavior as the
current catch path), and remove the try/catch; update the custom_field_value
function to handle the from_chars result and preserve the existing return
semantics.
| if (value.is_string()) { | ||
| const auto& str = value.get_ref<const std::string&>(); | ||
| if (str.empty()) { | ||
| return std::nullopt; | ||
| } | ||
| try { | ||
| std::size_t pos = 0; | ||
| double result = std::stod(str, &pos); | ||
| if (pos == str.size() && std::isfinite(result)) { | ||
| return result; | ||
| } | ||
| } catch (...) { | ||
| } | ||
| return std::nullopt; | ||
| } | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
std::from_chars double support compiler versions C++17 GCC Clang MSVC requirements
💡 Result:
std::from_chars for double (floating-point overloads) — practical support matrix (C++17)
| Toolchain | Standard library | Floating-point std::from_chars (incl. double) |
|---|---|---|
| GCC | libstdc++ | Implemented in libstdc++ trunk July 2020 (float/double/long double overloads were added then). (gcc.gnu.org) Later work switched float/double parsing to fast_float on IEEE binary32/64 targets. (gcc.gnu.org) |
| Clang | libc++ | libc++ reports: std::from_chars for float/double is available since libc++ v20; support is “complete except for long double”. (libcxx.llvm.org) |
| Clang | libstdc++ | Same as GCC’s libstdc++ version you’re linking against (so floating support depends on that libstdc++). (gcc.gnu.org) |
| MSVC | MSVC STL | Visual Studio 2017 15.8 implemented floating-point from_chars() in <charconv>. (devblogs.microsoft.com) |
Notes / requirements
- The floating overloads are the C++17 overloads in
<charconv>taking a floating type and an optionalstd::chars_format. (en.cppreference.com) - If you need a portable compile-time check, be careful: the usual feature-test macro situation has historically been imperfect in some libstdc++ configurations (headers/macros vs actual availability). (gcc.gnu.org)
Citations:
- 1: https://gcc.gnu.org/pipermail/libstdc%2B%2B/2020-July/050633.html?utm_source=openai
- 2: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588560.html?utm_source=openai
- 3: https://libcxx.llvm.org/Status/Cxx17.html?utm_source=openai
- 4: https://gcc.gnu.org/pipermail/libstdc%2B%2B/2020-July/050633.html?utm_source=openai
- 5: https://devblogs.microsoft.com/cppblog/stl-features-and-fixes-in-vs-2017-15-8/?utm_source=openai
- 6: https://en.cppreference.com/w/cpp/utility/from_chars.html?utm_source=openai
- 7: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591620.html?utm_source=openai
🌐 Web query:
std::stod locale sensitivity decimal separator C locale example
💡 Result:
std::stod is locale-sensitive because it is specified in terms of the C library conversion (strtod), which uses the C locale’s LC_NUMERIC category to decide what the decimal separator is. So, if your process locale uses a comma decimal point (e.g., many European locales), "1,5" may parse as 1.5 and "1.5" may stop at the . (or fail depending on input handling). (stackoverflow.com)
Minimal example (showing the effect)
`#include` <clocale>
`#include` <iostream>
`#include` <string>
int main() {
// Suppose your environment supports a comma-decimal locale like "de_DE.UTF-8".
std::setlocale(LC_NUMERIC, "de_DE.UTF-8");
std::cout << std::stod("1,5") << "\n"; // likely prints 1.5
// std::cout << std::stod("1.5") << "\n"; // may parse only "1" then stop at '.'
// Force "C" numeric rules: decimal point is '.'
std::setlocale(LC_NUMERIC, "C");
std::cout << std::stod("1.5") << "\n"; // prints 1.5
}LC_NUMERIC is the locale part that affects strtod/stod, and you can guarantee “C-locale” parsing by setting LC_NUMERIC (or LC_ALL) to "C". (open-std.org)
If you need locale-independent parsing (recommended)
- Prefer
std::from_charsfor floating-point (C++17/20; where implemented) since it’s not based on locales. - Or use a locale-fixed approach (e.g., a
std::istringstreamimbued withstd::locale::classic()), instead of changing the global C locale (which is process-global and problematic in multi-threaded code). (stackoverflow.com)
Citations:
- 1: https://stackoverflow.com/questions/12316972/how-to-tweak-stdstod-string-to-double-for-decimal-separator-and-number-of-di?utm_source=openai
- 2: https://www.open-std.org/jtc1/sc22/wg14/issues/c90/issue0024.html?utm_source=openai
- 3: https://stackoverflow.com/questions/12316972/how-to-tweak-stdstod-string-to-double-for-decimal-separator-and-number-of-di?utm_source=openai
Locale-sensitive string-to-number parsing may produce incorrect results.
std::stod respects the current C locale. If the application (or a third-party library) sets a locale that uses , as the decimal separator (e.g., de_DE), parsing "1.5" will yield 1.0 (stopping at the dot) and the pos == str.size() guard will return std::nullopt — silently breaking numeric string coercion. For an SDK that must behave deterministically, prefer a locale-independent approach.
🔧 Proposed fix using std::from_chars
std::from_chars (C++17, locale-independent by specification):
- try {
- std::size_t pos = 0;
- double result = std::stod(str, &pos);
- if (pos == str.size() && std::isfinite(result)) {
- return result;
- }
- } catch (...) {
- }
+ double result = 0.0;
+ auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), result);
+ if (ec == std::errc{} && ptr == str.data() + str.size() && std::isfinite(result)) {
+ return result;
+ }Compiler support: GCC 11+ (libstdc++), Clang 16+ (libc++), MSVC 19.15+ (Visual Studio 2017 15.8+).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (value.is_string()) { | |
| const auto& str = value.get_ref<const std::string&>(); | |
| if (str.empty()) { | |
| return std::nullopt; | |
| } | |
| try { | |
| std::size_t pos = 0; | |
| double result = std::stod(str, &pos); | |
| if (pos == str.size() && std::isfinite(result)) { | |
| return result; | |
| } | |
| } catch (...) { | |
| } | |
| return std::nullopt; | |
| } | |
| return std::nullopt; | |
| } | |
| if (value.is_string()) { | |
| const auto& str = value.get_ref<const std::string&>(); | |
| if (str.empty()) { | |
| return std::nullopt; | |
| } | |
| double result = 0.0; | |
| auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), result); | |
| if (ec == std::errc{} && ptr == str.data() + str.size() && std::isfinite(result)) { | |
| return result; | |
| } | |
| return std::nullopt; | |
| } | |
| return std::nullopt; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/json_expr/evaluator.cpp` around lines 84 - 100, The string-to-double
conversion uses locale-sensitive std::stod; replace it with locale-independent
std::from_chars (include <charconv>) to parse the std::string 'str' (or a
std::string_view) into a double named 'result', remove the try/catch and the pos
variable, then check that from_chars returned no error and consumed the entire
input (the end pointer equals str.data()+str.size()) and that
std::isfinite(result) before returning result; otherwise return std::nullopt —
update the conversion logic in the block that currently references value, str,
pos, result to use std::from_chars instead.
| if (value.is_number()) { | ||
| double d = value.get<double>(); | ||
| double int_part; | ||
| if (std::modf(d, &int_part) == 0.0 && std::abs(d) < 1e15) { | ||
| std::ostringstream oss; | ||
| oss << std::fixed << std::setprecision(0) << d; | ||
| return oss.str(); | ||
| } | ||
| std::ostringstream oss; | ||
| oss << std::setprecision(15) << d; | ||
| std::string result = oss.str(); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Locale-sensitive number-to-string formatting.
std::ostringstream uses the global locale by default. In locales with , as the decimal separator, to_string_value(1.5) would produce "1,5", breaking downstream string comparisons and audience matching. Imbue the stream with the classic "C" locale to ensure consistent output.
🔧 Proposed fix — imbue classic locale
if (value.is_number()) {
double d = value.get<double>();
double int_part;
if (std::modf(d, &int_part) == 0.0 && std::abs(d) < 1e15) {
std::ostringstream oss;
+ oss.imbue(std::locale::classic());
oss << std::fixed << std::setprecision(0) << d;
return oss.str();
}
std::ostringstream oss;
+ oss.imbue(std::locale::classic());
oss << std::setprecision(15) << d;
std::string result = oss.str();
return result;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/json_expr/evaluator.cpp` around lines 112 - 124, The number-to-string
path in src/json_expr/evaluator.cpp uses std::ostringstream (the two local
variables named oss) which pick up the global locale; imbue each ostringstream
with the C locale (call oss.imbue(std::locale::classic()) immediately after
construction) before formatting so decimal separators are always '.'; also
ensure <locale> is included if not already.
| for (; w < 16; ++w) { | ||
| block[w] = 0; | ||
| } | ||
|
|
||
| block[14] = static_cast<uint32_t>(len << 3); | ||
| md5cycle(state, block); |
There was a problem hiding this comment.
Encode the MD5 bit‑length as 64‑bit to avoid truncation.
On Line 173 the length is written as a 32‑bit value, so inputs ≥ 512 MB will hash incorrectly. MD5 appends a 64‑bit bit‑length; write both low and high words.
🔧 Suggested update
- block[14] = static_cast<uint32_t>(len << 3);
- md5cycle(state, block);
+ const uint64_t bit_len = static_cast<uint64_t>(len) << 3;
+ block[14] = static_cast<uint32_t>(bit_len & 0xFFFFFFFFu);
+ block[15] = static_cast<uint32_t>(bit_len >> 32);
+ md5cycle(state, block);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (; w < 16; ++w) { | |
| block[w] = 0; | |
| } | |
| block[14] = static_cast<uint32_t>(len << 3); | |
| md5cycle(state, block); | |
| for (; w < 16; ++w) { | |
| block[w] = 0; | |
| } | |
| const uint64_t bit_len = static_cast<uint64_t>(len) << 3; | |
| block[14] = static_cast<uint32_t>(bit_len & 0xFFFFFFFFu); | |
| block[15] = static_cast<uint32_t>(bit_len >> 32); | |
| md5cycle(state, block); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/md5.cpp` around lines 169 - 174, The code currently writes only a 32-bit
length into block[14], truncating inputs ≥512MB; compute the full 64-bit
bit-length (e.g. uint64_t bitlen = static_cast<uint64_t>(len) << 3), then store
the low 32 bits into block[14] and the high 32 bits into block[15] before
calling md5cycle(state, block) so the MD5 length is encoded as a 64-bit value.
Add SDK::create() factory and sdk->create_context() / create_context_with() convenience methods for simpler initialization.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
README.md (2)
11-37:⚠️ Potential issue | 🟡 MinorFix heading level jumps in the Installation section.
Lines 11, 28, and 37 jump from H2 to H4, which violates markdownlint's heading increment rule. These subsection headings should be H3.
📝 Proposed change
-#### CMake (FetchContent) +### CMake (FetchContent) Add the following to your `CMakeLists.txt` to include the SDK directly from the repository: @@ -25,7 +25,7 @@ target_link_libraries(your_target PRIVATE absmartly-sdk)-#### CMake (Local)
+### CMake (Local)If you have cloned the repository locally, you can add it as a subdirectory:
@@ -34,7 +34,7 @@
target_link_libraries(your_target PRIVATE absmartly-sdk)-#### Building from Source +### Building from Source🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 11 - 37, Change the three subsection headings currently using H4 (the lines titled "CMake (FetchContent)", "CMake (Local)", and "Building from Source") to H3 so they consistently increment from the H2 "Installation" heading; update the heading markers from #### to ### for the symbols "CMake (FetchContent)", "CMake (Local)", and "Building from Source" and verify the surrounding blocks keep the same content and spacing.
238-238:⚠️ Potential issue | 🟡 MinorAdd comma for improved readability in compound sentence.
The sentence connects two independent clauses and would read more smoothly with a comma before "and".
📝 Proposed change
-You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in and you want to associate the new identity with the context. +You can add additional units to a context by calling the `set_unit()` or `set_units()` methods. This is useful when a user logs in, and you want to associate the new identity with the context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 238, Insert a comma before the conjunction "and" in the sentence "This is useful when a user logs in and you want to associate the new identity with the context." so it reads "This is useful when a user logs in, and you want to associate the new identity with the context." to improve readability; update the README.md sentence accordingly.
🧹 Nitpick comments (7)
README.md (1)
21-21: Consider recommending a pinned version instead of "main".Using
GIT_TAG mainin the FetchContent example will fetch the latest commit from the main branch, which could introduce breaking changes unexpectedly and makes builds non-reproducible. For production use, it's better to recommend a specific tag or commit SHA.📌 Suggested alternative
FetchContent_Declare( absmartly-sdk GIT_REPOSITORY https://github.com/absmartly/cpp-sdk.git - GIT_TAG main + GIT_TAG v1.0.0 # or a specific commit SHA )Alternatively, add a note in the documentation explaining that users should replace
mainwith a specific version tag for production use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 21, The README uses an unstable branch reference "GIT_TAG main" which risks non-reproducible builds; update the documentation to recommend pinning to a specific tag or commit (e.g., replace "GIT_TAG main" with a version tag or commit SHA) and add a brief note advising users to use a fixed tag/sha for production to ensure reproducible builds.CMakeLists.txt (2)
4-5: Consider setting C++ standard per-target instead of globally.Setting
CMAKE_CXX_STANDARDglobally affects all targets in the project. For better encapsulation, consider usingtarget_compile_features(absmartly-sdk PUBLIC cxx_std_17)instead, which also ensures consumers use C++17.♻️ Proposed refactor
-set(CMAKE_CXX_STANDARD 17) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +# C++ standard set per-target belowThen after
add_library:target_link_libraries(absmartly-sdk PUBLIC nlohmann_json::nlohmann_json) +target_compile_features(absmartly-sdk PUBLIC cxx_std_17)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 4 - 5, The CMake file currently sets C++17 globally via CMAKE_CXX_STANDARD; instead, remove or stop relying on the global setting and set the standard per-target by adding target_compile_features(absmartly-sdk PUBLIC cxx_std_17) immediately after the add_library(...) that defines the absmartly-sdk target, so the absmartly-sdk target and its consumers are required to use C++17 without impacting other targets.
47-47: Consider defaultingABSMARTLY_BUILD_TESTSto OFF.Having tests enabled by default means consumers who use
add_subdirectoryor FetchContent will build tests unnecessarily. A common convention is to default tests to OFF for library projects, or enable them only when built as the top-level project.♻️ Proposed refactor
-option(ABSMARTLY_BUILD_TESTS "Build tests" ON) +option(ABSMARTLY_BUILD_TESTS "Build tests" OFF) + +# Or conditionally enable when this is the main project: +# if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) +# set(ABSMARTLY_BUILD_TESTS_DEFAULT ON) +# else() +# set(ABSMARTLY_BUILD_TESTS_DEFAULT OFF) +# endif() +# option(ABSMARTLY_BUILD_TESTS "Build tests" ${ABSMARTLY_BUILD_TESTS_DEFAULT})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 47, Change the option ABSMARTLY_BUILD_TESTS to default OFF instead of ON; update the CMakeLists to declare option(ABSMARTLY_BUILD_TESTS "Build tests" OFF) and, if you want to keep the old top-level behavior, wrap a top-level check such as comparing CMAKE_SOURCE_DIR and CMAKE_CURRENT_SOURCE_DIR to set it to ON only when the project is the top-level build (or conditionally add tests only when that variable is true). Ensure references to ABSMARTLY_BUILD_TESTS in add_subdirectory/test-target logic remain unchanged.include/absmartly/context.h (2)
41-44: Document ownership semantics for raw pointer inExperimentIndex.
ExperimentIndex::datais a raw pointer toExperimentData. The ownership relationship should be documented—presumably this points intodata_.experimentsand is only valid whilst the parentContextis alive. Consider adding a comment to clarify this invariant.📝 Suggested documentation
struct ExperimentIndex { - const ExperimentData* data = nullptr; + const ExperimentData* data = nullptr; // Non-owning; points into Context::data_.experiments std::vector<std::map<std::string, nlohmann::json>> variables; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/context.h` around lines 41 - 44, Document that ExperimentIndex::data is a non-owning pointer: add a concise comment on the ExperimentIndex struct stating that data is a raw, non-owning pointer to an ExperimentData that lives inside the parent Context (e.g., Context::data_.experiments) and is only valid while that Context remains alive; mention the invariant that ExperimentIndex instances must not outlive their owning Context and that no deletion is performed through this pointer.
12-12:<set>header appears unused in this header file.The
<set>include is not used in any declarations here. If it is only needed in the implementation (src/context.cpp), move it there to reduce header dependencies and compilation times.♻️ Proposed fix
`#include` <map> -#include <set> `#include` <vector>Then add
#include <set>insrc/context.cppif needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/context.h` at line 12, Remove the unused `#include` <set> from the context.h header and, if any std::set usage exists in the implementation of the context declarations, add `#include` <set> to the corresponding source file that implements those functions (the context.cpp implementation). This reduces header dependencies and keeps the header minimal.src/context.cpp (2)
14-20: Floating-point exact equality comparison may be fragile.
array_equals_shallowuses==to comparedoublevalues. While this may be intentional for comparing stored traffic splits that originated from the same source, floating-point exact equality can be problematic if values are ever computed differently. Consider whether an epsilon-based comparison is warranted, or add a comment explaining why exact equality is safe here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 14 - 20, The function array_equals_shallow uses exact equality on doubles which is brittle; update array_equals_shallow to perform comparisons with a small relative/absolute epsilon (or use std::fabs(a[i]-b[i]) <= epsilon) to tolerate floating-point rounding, and choose/define a sensible EPSILON constant nearby (or accept an optional tolerance parameter); alternatively, if exact bitwise equality is intended, add a clear comment in array_equals_shallow explaining why exact equality is safe and under what guarantees (e.g., values always come from the same source) so future maintainers won’t change it.
56-59: Redundant assignment ofdata_ininit().
become_ready()already movesdataintodata_at line 57, then callsinit(data_)which assignsdata_again at line 618. The second assignment is redundant.♻️ Proposed fix
void Context::init(const ContextData& data) { - data_ = data; build_index(); }Or alternatively, if
initis intended to be callable independently:void Context::become_ready(ContextData data) { - data_ = std::move(data); - init(data_); + init(std::move(data)); ready_ = true; // ... } -void Context::init(const ContextData& data) { - data_ = data; +void Context::init(ContextData data) { + data_ = std::move(data); build_index(); }Also applies to: 617-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 56 - 59, Context::become_ready currently moves the incoming ContextData into data_ then calls init(data_), but init also reassigns data_ (redundant); remove the duplicate assignment inside Context::init (the data_ = std::move(data) or similar) and have init operate on the already-stored data_ (or change init to take no parameter), leaving Context::become_ready as data_ = std::move(data); init(); ready_ = true; so the double move/assignment is eliminated; update references to the parameter inside Context::init accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/absmartly/sdk.h`:
- Around line 1-7: Add the missing <future> include so std::future is available:
update the header (include/absmartly/sdk.h) to `#include` <future> near the other
includes, ensuring the declaration std::future<ContextData> get_context_data()
compiles; keep the include order consistent with existing standard headers and
avoid adding other code changes.
- Line 3: The SDK references an undefined type SDKConfig and includes a missing
header; create a new header that defines the SDKConfig type (e.g., a simple
struct with the configuration fields used by class SDK and any defaults),
protect it with include guards or `#pragma` once, and ensure it declares anything
the SDK methods (constructor/factory methods) expect; then keep the existing
`#include` <absmartly/sdk_config.h> in the SDK header so SDK can compile. If you
prefer not to add a header, remove the `#include` and replace SDKConfig usages
inside class SDK with a suitable existing type or a forward declaration and
update the SDK methods (e.g., SDK::create, SDK constructor) accordingly.
In `@README.md`:
- Around line 455-469: The README links for each SDK (e.g., link items like
"[Java SDK]", "[JavaScript SDK]", "[C++ SDK]" etc.) use non-canonical URLs with
the "www" subdomain (https://www.github.com/...). Update each SDK link target to
use the canonical GitHub domain by removing "www" so they read
https://github.com/absmartly/<repo>, ensuring every listed link (Java,
JavaScript, PHP, Swift, Vue2, Vue3, React, Angular, Python3, Go, Ruby, .NET,
C++, Dart, Flutter) is changed accordingly.
In `@src/context.cpp`:
- Around line 70-88: The wait_until_ready() implementation calls
data_future_.get() which is single-use; change data_future_ from std::future<T>
to std::shared_future<T> (update its declaration and wherever it is
assigned/constructed) so multiple calls to Context::wait_until_ready() are safe,
then replace .get() usage in wait_until_ready() with the shared_future's .get();
ensure constructors or factory functions that produce the future convert or call
.share() to initialize data_future_, and keep existing logic around ready_,
failed_, become_ready, and emit_event unchanged.
- Around line 644-645: The silent catch(...) in src/context.cpp that wraps
parsing of variant.config should be replaced with a typed catch that logs the
error and the offending config so parse failures are discoverable: catch
std::exception (e.g., catch (const std::exception& e)) and call your existing
logging/emitter (e.g., processLogger.warn or emitEvent) including e.what() and
the raw variant.config (and variant identifier if available); if non-std
exceptions must be handled too, add a final catch(...) that logs a generic parse
failure. Ensure the log message clearly names the operation (parsing
variant.config) and includes the value and error details.
- Around line 382-418: The Context class (methods like publish(), and members
pending_, exposures_, goals_, attrs_, units_) is not thread-safe; either add
synchronization or document single-threaded usage: to fix, add a std::mutex
member to Context (e.g., mutex_), protect all accesses and mutations of
pending_, exposures_, goals_, attrs_, and units_ by locking (use
std::lock_guard<std::mutex> or std::scoped_lock) in publish() and any other
methods that read/write these members, or alternatively add a clear comment in
the class header and README stating Context must only be used from a single
thread and ensure all call sites are single-threaded.
---
Duplicate comments:
In `@README.md`:
- Around line 11-37: Change the three subsection headings currently using H4
(the lines titled "CMake (FetchContent)", "CMake (Local)", and "Building from
Source") to H3 so they consistently increment from the H2 "Installation"
heading; update the heading markers from #### to ### for the symbols "CMake
(FetchContent)", "CMake (Local)", and "Building from Source" and verify the
surrounding blocks keep the same content and spacing.
- Line 238: Insert a comma before the conjunction "and" in the sentence "This is
useful when a user logs in and you want to associate the new identity with the
context." so it reads "This is useful when a user logs in, and you want to
associate the new identity with the context." to improve readability; update the
README.md sentence accordingly.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 4-5: The CMake file currently sets C++17 globally via
CMAKE_CXX_STANDARD; instead, remove or stop relying on the global setting and
set the standard per-target by adding target_compile_features(absmartly-sdk
PUBLIC cxx_std_17) immediately after the add_library(...) that defines the
absmartly-sdk target, so the absmartly-sdk target and its consumers are required
to use C++17 without impacting other targets.
- Line 47: Change the option ABSMARTLY_BUILD_TESTS to default OFF instead of ON;
update the CMakeLists to declare option(ABSMARTLY_BUILD_TESTS "Build tests" OFF)
and, if you want to keep the old top-level behavior, wrap a top-level check such
as comparing CMAKE_SOURCE_DIR and CMAKE_CURRENT_SOURCE_DIR to set it to ON only
when the project is the top-level build (or conditionally add tests only when
that variable is true). Ensure references to ABSMARTLY_BUILD_TESTS in
add_subdirectory/test-target logic remain unchanged.
In `@include/absmartly/context.h`:
- Around line 41-44: Document that ExperimentIndex::data is a non-owning
pointer: add a concise comment on the ExperimentIndex struct stating that data
is a raw, non-owning pointer to an ExperimentData that lives inside the parent
Context (e.g., Context::data_.experiments) and is only valid while that Context
remains alive; mention the invariant that ExperimentIndex instances must not
outlive their owning Context and that no deletion is performed through this
pointer.
- Line 12: Remove the unused `#include` <set> from the context.h header and, if
any std::set usage exists in the implementation of the context declarations, add
`#include` <set> to the corresponding source file that implements those functions
(the context.cpp implementation). This reduces header dependencies and keeps the
header minimal.
In `@README.md`:
- Line 21: The README uses an unstable branch reference "GIT_TAG main" which
risks non-reproducible builds; update the documentation to recommend pinning to
a specific tag or commit (e.g., replace "GIT_TAG main" with a version tag or
commit SHA) and add a brief note advising users to use a fixed tag/sha for
production to ensure reproducible builds.
In `@src/context.cpp`:
- Around line 14-20: The function array_equals_shallow uses exact equality on
doubles which is brittle; update array_equals_shallow to perform comparisons
with a small relative/absolute epsilon (or use std::fabs(a[i]-b[i]) <= epsilon)
to tolerate floating-point rounding, and choose/define a sensible EPSILON
constant nearby (or accept an optional tolerance parameter); alternatively, if
exact bitwise equality is intended, add a clear comment in array_equals_shallow
explaining why exact equality is safe and under what guarantees (e.g., values
always come from the same source) so future maintainers won’t change it.
- Around line 56-59: Context::become_ready currently moves the incoming
ContextData into data_ then calls init(data_), but init also reassigns data_
(redundant); remove the duplicate assignment inside Context::init (the data_ =
std::move(data) or similar) and have init operate on the already-stored data_
(or change init to take no parameter), leaving Context::become_ready as data_ =
std::move(data); init(); ready_ = true; so the double move/assignment is
eliminated; update references to the parameter inside Context::init accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txtREADME.mdinclude/absmartly/context.hinclude/absmartly/sdk.hsrc/context.cpp
| #pragma once | ||
|
|
||
| #include <absmartly/sdk_config.h> | ||
| #include <absmartly/context.h> | ||
| #include <absmartly/context_config.h> | ||
| #include <memory> | ||
|
|
There was a problem hiding this comment.
Missing <future> header for std::future usage.
Line 17 declares std::future<ContextData> get_context_data() but <future> is not included. This will cause a compilation error.
🐛 Proposed fix
`#include` <absmartly/sdk_config.h>
`#include` <absmartly/context.h>
`#include` <absmartly/context_config.h>
`#include` <memory>
+#include <future>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #pragma once | |
| #include <absmartly/sdk_config.h> | |
| #include <absmartly/context.h> | |
| #include <absmartly/context_config.h> | |
| #include <memory> | |
| `#pragma` once | |
| `#include` <absmartly/sdk_config.h> | |
| `#include` <absmartly/context.h> | |
| `#include` <absmartly/context_config.h> | |
| `#include` <memory> | |
| `#include` <future> | |
🧰 Tools
🪛 Clang (14.0.6)
[error] 3-3: 'absmartly/sdk_config.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/absmartly/sdk.h` around lines 1 - 7, Add the missing <future> include
so std::future is available: update the header (include/absmartly/sdk.h) to
`#include` <future> near the other includes, ensuring the declaration
std::future<ContextData> get_context_data() compiles; keep the include order
consistent with existing standard headers and avoid adding other code changes.
| - [Java SDK](https://www.github.com/absmartly/java-sdk) | ||
| - [JavaScript SDK](https://www.github.com/absmartly/javascript-sdk) | ||
| - [PHP SDK](https://www.github.com/absmartly/php-sdk) | ||
| - [Swift SDK](https://www.github.com/absmartly/swift-sdk) | ||
| - [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk) | ||
| - [Vue3 SDK](https://www.github.com/absmartly/vue3-sdk) | ||
| - [React SDK](https://www.github.com/absmartly/react-sdk) | ||
| - [Angular SDK](https://www.github.com/absmartly/angular-sdk) | ||
| - [Python3 SDK](https://www.github.com/absmartly/python3-sdk) | ||
| - [Go SDK](https://www.github.com/absmartly/go-sdk) | ||
| - [Ruby SDK](https://www.github.com/absmartly/ruby-sdk) | ||
| - [.NET SDK](https://www.github.com/absmartly/dotnet-sdk) | ||
| - [C++ SDK](https://www.github.com/absmartly/cpp-sdk) (this package) | ||
| - [Dart SDK](https://www.github.com/absmartly/dart-sdk) | ||
| - [Flutter SDK](https://www.github.com/absmartly/flutter-sdk) |
There was a problem hiding this comment.
Use canonical GitHub URLs without "www" subdomain.
All SDK links include the "www" subdomain (https://www.github.com/...), but GitHub's canonical domain is github.com without "www". Whilst the links work due to redirects, it's better to use the canonical form.
🔗 Proposed change
-### Have a look at our growing list of clients and SDKs:
-- [Java SDK](https://www.github.com/absmartly/java-sdk)
-- [JavaScript SDK](https://www.github.com/absmartly/javascript-sdk)
-- [PHP SDK](https://www.github.com/absmartly/php-sdk)
-- [Swift SDK](https://www.github.com/absmartly/swift-sdk)
-- [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk)
-- [Vue3 SDK](https://www.github.com/absmartly/vue3-sdk)
-- [React SDK](https://www.github.com/absmartly/react-sdk)
-- [Angular SDK](https://www.github.com/absmartly/angular-sdk)
-- [Python3 SDK](https://www.github.com/absmartly/python3-sdk)
-- [Go SDK](https://www.github.com/absmartly/go-sdk)
-- [Ruby SDK](https://www.github.com/absmartly/ruby-sdk)
-- [.NET SDK](https://www.github.com/absmartly/dotnet-sdk)
-- [C++ SDK](https://www.github.com/absmartly/cpp-sdk) (this package)
-- [Dart SDK](https://www.github.com/absmartly/dart-sdk)
-- [Flutter SDK](https://www.github.com/absmartly/flutter-sdk)
+### Have a look at our growing list of clients and SDKs:
+- [Java SDK](https://github.com/absmartly/java-sdk)
+- [JavaScript SDK](https://github.com/absmartly/javascript-sdk)
+- [PHP SDK](https://github.com/absmartly/php-sdk)
+- [Swift SDK](https://github.com/absmartly/swift-sdk)
+- [Vue2 SDK](https://github.com/absmartly/vue2-sdk)
+- [Vue3 SDK](https://github.com/absmartly/vue3-sdk)
+- [React SDK](https://github.com/absmartly/react-sdk)
+- [Angular SDK](https://github.com/absmartly/angular-sdk)
+- [Python3 SDK](https://github.com/absmartly/python3-sdk)
+- [Go SDK](https://github.com/absmartly/go-sdk)
+- [Ruby SDK](https://github.com/absmartly/ruby-sdk)
+- [.NET SDK](https://github.com/absmartly/dotnet-sdk)
+- [C++ SDK](https://github.com/absmartly/cpp-sdk) (this package)
+- [Dart SDK](https://github.com/absmartly/dart-sdk)
+- [Flutter SDK](https://github.com/absmartly/flutter-sdk)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [Java SDK](https://www.github.com/absmartly/java-sdk) | |
| - [JavaScript SDK](https://www.github.com/absmartly/javascript-sdk) | |
| - [PHP SDK](https://www.github.com/absmartly/php-sdk) | |
| - [Swift SDK](https://www.github.com/absmartly/swift-sdk) | |
| - [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk) | |
| - [Vue3 SDK](https://www.github.com/absmartly/vue3-sdk) | |
| - [React SDK](https://www.github.com/absmartly/react-sdk) | |
| - [Angular SDK](https://www.github.com/absmartly/angular-sdk) | |
| - [Python3 SDK](https://www.github.com/absmartly/python3-sdk) | |
| - [Go SDK](https://www.github.com/absmartly/go-sdk) | |
| - [Ruby SDK](https://www.github.com/absmartly/ruby-sdk) | |
| - [.NET SDK](https://www.github.com/absmartly/dotnet-sdk) | |
| - [C++ SDK](https://www.github.com/absmartly/cpp-sdk) (this package) | |
| - [Dart SDK](https://www.github.com/absmartly/dart-sdk) | |
| - [Flutter SDK](https://www.github.com/absmartly/flutter-sdk) | |
| ### Have a look at our growing list of clients and SDKs: | |
| - [Java SDK](https://github.com/absmartly/java-sdk) | |
| - [JavaScript SDK](https://github.com/absmartly/javascript-sdk) | |
| - [PHP SDK](https://github.com/absmartly/php-sdk) | |
| - [Swift SDK](https://github.com/absmartly/swift-sdk) | |
| - [Vue2 SDK](https://github.com/absmartly/vue2-sdk) | |
| - [Vue3 SDK](https://github.com/absmartly/vue3-sdk) | |
| - [React SDK](https://github.com/absmartly/react-sdk) | |
| - [Angular SDK](https://github.com/absmartly/angular-sdk) | |
| - [Python3 SDK](https://github.com/absmartly/python3-sdk) | |
| - [Go SDK](https://github.com/absmartly/go-sdk) | |
| - [Ruby SDK](https://github.com/absmartly/ruby-sdk) | |
| - [.NET SDK](https://github.com/absmartly/dotnet-sdk) | |
| - [C++ SDK](https://github.com/absmartly/cpp-sdk) (this package) | |
| - [Dart SDK](https://github.com/absmartly/dart-sdk) | |
| - [Flutter SDK](https://github.com/absmartly/flutter-sdk) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 455 - 469, The README links for each SDK (e.g., link
items like "[Java SDK]", "[JavaScript SDK]", "[C++ SDK]" etc.) use non-canonical
URLs with the "www" subdomain (https://www.github.com/...). Update each SDK link
target to use the canonical GitHub domain by removing "www" so they read
https://github.com/absmartly/<repo>, ensuring every listed link (Java,
JavaScript, PHP, Swift, Vue2, Vue3, React, Angular, Python3, Go, Ruby, .NET,
C++, Dart, Flutter) is changed accordingly.
| void Context::wait_until_ready() { | ||
| if (ready_ || failed_) { | ||
| return; | ||
| } | ||
|
|
||
| if (!data_future_.valid()) { | ||
| failed_ = true; | ||
| emit_event("error", {{"message", "No data future available"}}); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| auto data = data_future_.get(); | ||
| become_ready(std::move(data)); | ||
| } catch (const std::exception& e) { | ||
| failed_ = true; | ||
| emit_event("error", {{"message", e.what()}}); | ||
| } | ||
| } |
There was a problem hiding this comment.
wait_until_ready() can only be called once due to std::future::get() semantics.
std::future::get() can only be called once; subsequent calls result in undefined behaviour or an exception (depending on implementation). If wait_until_ready() is called twice before the future resolves, the second call will fail. Consider using std::shared_future if multiple waits are expected, or document that this method must only be called once.
♻️ Alternative using shared_future
In header:
- std::future<ContextData> data_future_;
+ std::shared_future<ContextData> data_future_;In constructor:
- , data_future_(std::move(data_future)) {
+ , data_future_(data_future.share()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.cpp` around lines 70 - 88, The wait_until_ready() implementation
calls data_future_.get() which is single-use; change data_future_ from
std::future<T> to std::shared_future<T> (update its declaration and wherever it
is assigned/constructed) so multiple calls to Context::wait_until_ready() are
safe, then replace .get() usage in wait_until_ready() with the shared_future's
.get(); ensure constructors or factory functions that produce the future convert
or call .share() to initialize data_future_, and keep existing logic around
ready_, failed_, become_ready, and emit_event unchanged.
| PublishEvent Context::publish() { | ||
| if (pending_ == 0) { | ||
| return {}; | ||
| } | ||
|
|
||
| PublishEvent event; | ||
| event.hashed = true; | ||
| event.publishedAt = now_millis(); | ||
|
|
||
| for (const auto& [type, uid] : units_) { | ||
| Unit u; | ||
| u.type = type; | ||
| u.uid = unit_hash(type); | ||
| event.units.push_back(std::move(u)); | ||
| } | ||
|
|
||
| if (!exposures_.empty()) { | ||
| event.exposures = std::move(exposures_); | ||
| exposures_.clear(); | ||
| } | ||
|
|
||
| if (!goals_.empty()) { | ||
| event.goals = std::move(goals_); | ||
| goals_.clear(); | ||
| } | ||
|
|
||
| if (!attrs_.empty()) { | ||
| event.attributes = attrs_; | ||
| } | ||
|
|
||
| pending_ = 0; | ||
|
|
||
| nlohmann::json pub_data = event; | ||
| emit_event("publish", pub_data); | ||
|
|
||
| return event; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Context class definition
fd -t f -e h context.h | head -5Repository: absmartly/cpp-sdk
Length of output: 88
🏁 Script executed:
# Search for thread-related documentation and synchronisation keywords
rg -i "thread|mutex|lock|atomic" include/absmartly/ --type cpp --type hRepository: absmartly/cpp-sdk
Length of output: 43
🏁 Script executed:
# Examine the full context.h file to understand the class structure
cat include/absmartly/context.hRepository: absmartly/cpp-sdk
Length of output: 5254
🏁 Script executed:
# Look at the context.cpp file around the publish() method and check for any synchronisation
sed -n '1,50p' src/context.cppRepository: absmartly/cpp-sdk
Length of output: 1650
🏁 Script executed:
# Check if there's a README or documentation about thread-safety
find . -maxdepth 2 -name "README*" -o -name "*.md" | head -10Repository: absmartly/cpp-sdk
Length of output: 72
🏁 Script executed:
# Read the README to check for thread-safety documentation
cat README.mdRepository: absmartly/cpp-sdk
Length of output: 16471
🏁 Script executed:
# Search more broadly in the entire src/context.cpp file for synchronisation
rg "mutex|lock|atomic|thread" src/context.cppRepository: absmartly/cpp-sdk
Length of output: 109
🏁 Script executed:
# Check how Context is constructed and used in tests/examples
fd -t f -e cpp -e h test examples | head -20Repository: absmartly/cpp-sdk
Length of output: 156
🏁 Script executed:
# Search for any documentation comments in context.cpp about thread-safety
rg -B5 -A5 "publish\(" src/context.cpp | head -40Repository: absmartly/cpp-sdk
Length of output: 406
🏁 Script executed:
# Check if there are any member variables that suggest thread-safety (like mutex)
rg "std::mutex|std::lock|std::atomic" include/absmartly/Repository: absmartly/cpp-sdk
Length of output: 43
publish() (and the entire Context class) is not thread-safe.
publish() and other methods read and modify shared state (pending_, exposures_, goals_, attrs_, units_) without synchronisation. The class contains no synchronisation primitives (no std::mutex, std::atomic, or locks). Data races will occur if Context is accessed concurrently from multiple threads. Either:
- Add thread-safe synchronisation if multi-threaded use is intended, or
- Document prominently that
Contextis not thread-safe and must be used from a single thread only.
The README provides no guidance on this constraint, and the usage examples suggest single-threaded usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.cpp` around lines 382 - 418, The Context class (methods like
publish(), and members pending_, exposures_, goals_, attrs_, units_) is not
thread-safe; either add synchronization or document single-threaded usage: to
fix, add a std::mutex member to Context (e.g., mutex_), protect all accesses and
mutations of pending_, exposures_, goals_, attrs_, and units_ by locking (use
std::lock_guard<std::mutex> or std::scoped_lock) in publish() and any other
methods that read/write these members, or alternatively add a clear comment in
the class header and README stating Context must only be used from a single
thread and ensure all call sites are single-threaded.
| } catch (...) { | ||
| } |
There was a problem hiding this comment.
Silent catch block hides JSON parse errors.
If variant.config contains invalid JSON, the error is silently ignored. Consider logging a warning or emitting an event so configuration issues are discoverable during debugging.
🛠️ Suggested improvement
try {
auto config_json = nlohmann::json::parse(config_str);
if (config_json.is_object()) {
for (auto it = config_json.begin(); it != config_json.end(); ++it) {
parsed[it.key()] = it.value();
}
}
- } catch (...) {
+ } catch (const std::exception& e) {
+ // Optionally log or emit an event about malformed config
+ // e.g., emit_event("warning", {{"message", "Failed to parse variant config"}, {"error", e.what()}});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.cpp` around lines 644 - 645, The silent catch(...) in
src/context.cpp that wraps parsing of variant.config should be replaced with a
typed catch that logs the error and the offending config so parse failures are
discoverable: catch std::exception (e.g., catch (const std::exception& e)) and
call your existing logging/emitter (e.g., processLogger.warn or emitEvent)
including e.what() and the raw variant.config (and variant identifier if
available); if non-std exceptions must be handled too, add a final catch(...)
that logs a generic parse failure. Ensure the log message clearly names the
operation (parsing variant.config) and includes the value and error details.
- Add AsyncHTTPClient using curl_multi with single background event loop thread - Add HTTPMode enum for sync vs async selection in create_simple() - Make CURL a required dependency (needed for both sync and async clients) - Add non-blocking is_ready() check using wait_for(0s) on data future - Add SDK wrapper pattern with Client, DefaultContextDataProvider, DefaultContextEventPublisher, and create_simple convenience methods
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
src/context.cpp (1)
202-209:⚠️ Potential issue | 🟡 MinorFreeze overrides after finalisation as well.
set_custom_assignment()already callscheck_not_finalized(), butset_override()still mutatesoverrides_after finalisation. That leaves two neighbouring mutation APIs with different lifecycle rules for no obvious reason.Possible fix
void Context::set_override(const std::string& experiment_name, int variant) { + check_not_finalized(); overrides_[experiment_name] = variant; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.cpp` around lines 202 - 209, set_override currently mutates overrides_ after finalization; to match set_custom_assignment behavior, call check_not_finalized() at the start of set_override before updating overrides_, and also call check_not_finalized() at the start of set_overrides (or rely on the per-item set_override check but prefer a single pre-check) to avoid partial application; reference functions: set_override, set_overrides, set_custom_assignment and member overrides_.
🧹 Nitpick comments (1)
include/absmartly/default_http_client.h (1)
31-36: Extract the shared curl helpers before the two clients drift.
build_url_with_query,build_header_list, andwrite_callbackare already mirrored ininclude/absmartly/async_http_client.h. Centralising them in one internal utility would avoid maintaining two copies of the same request-building logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/absmartly/default_http_client.h` around lines 31 - 36, Extract the duplicated cURL helper functions build_url_with_query, build_header_list and write_callback out of the two client headers and into a single internal utility (e.g., an internal namespace/header + source file); remove the per-header static duplicates, put shared declarations in the new header and their implementations in the new .cpp, adjust both DefaultHttpClient::perform_request and AsyncHttpClient usages to call the shared functions (or qualify them with the internal namespace), and ensure build_header_list returns/cleans a curl_slist and write_callback has the same signature and linkage used by perform_request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 37-39: The CMake target_link_libraries call links CURL::libcurl as
PRIVATE for target absmartly-sdk; change that linkage to PUBLIC so the curl
dependency propagates to consumers (i.e., replace the PRIVATE specifier with
PUBLIC in the target_link_libraries(absmartly-sdk ... CURL::libcurl) invocation
while keeping find_package(CURL REQUIRED) and the existing target_sources calls
intact).
In `@include/absmartly/default_context_data_provider.h`:
- Around line 11-16: The DefaultContextDataProvider constructor currently
accepts a null std::shared_ptr<Client>, which later causes a null dereference in
get_context_data; update the constructor
(DefaultContextDataProvider(std::shared_ptr<Client> client)) to validate that
client is not null and reject it (e.g., throw std::invalid_argument or use an
assert/Expects) and store the validated pointer in client_; ensure the check is
performed in the constructor implementation so client_->get_context_data() in
get_context_data() is safe.
In `@include/absmartly/default_context_event_publisher.h`:
- Around line 11-16: The constructor for DefaultContextEventPublisher must
reject null Client instances because publish() dereferences client_
unconditionally; update the constructor (DefaultContextEventPublisher) to
validate the passed std::shared_ptr<Client> and throw (e.g.,
std::invalid_argument) if it's null, or change the member/type to a non-nullable
alternative (e.g., store a Client& or use a not-null wrapper) so client_ can
never be null; ensure the change prevents construction with nullptr and keeps
publish(const PublishEvent&) working without additional null checks.
In `@src/async_http_client.cpp`:
- Around line 13-31: Destructor currently only cleans active_requests_ leaving
PendingRequest objects in pending_queue_ leaked and their promises unresolved;
update AsyncHTTPClient::~AsyncHTTPClient to drain pending_queue_ during
shutdown: after setting running_ = false and notifying pending_cv_, acquire the
pending queue mutex (pending_mutex_), iterate/popleft each PendingRequest in
pending_queue_, free any req->headers (curl_slist_free_all), set the
req->promise with a cancellation result (e.g., status 0 and "Client shutting
down"), delete the PendingRequest, and clear the queue before proceeding to
clean active_requests_ and join; this ensures all pending futures are completed
and no leaks remain.
- Around line 7-10: The AsyncHTTPClient constructor must check libcurl init
return values and throw before starting worker_; call curl_global_init and
verify its return code is CURLE_OK, then call curl_multi_init and ensure the
returned multi_ is non-null; if curl_multi_init fails unwind by calling
curl_global_cleanup() and throw a std::runtime_error (include <stdexcept> at
top) so the worker_ thread is not spawned, and only start worker_ =
std::thread(&AsyncHTTPClient::event_loop, this) after both inits succeed.
In `@src/client.cpp`:
- Around line 8-9: The constructor Client::Client should reject a null HTTP
transport because get_context_data() and publish() dereference http_client_
unconditionally; add a null check in Client::Client(const ClientConfig&,
std::shared_ptr<HTTPClient>) that validates the incoming http_client parameter
and throws a clear exception (e.g., std::invalid_argument or std::runtime_error
with a descriptive message) if it's null before assigning http_client_ (or use
an assert if project policy prefers); this ensures clients cannot be constructed
with a null http_client_ and prevents crashes in get_context_data() and
publish().
In `@src/context.cpp`:
- Around line 393-443: Context::publish and Context::finalize currently build a
PublishEvent but never forward it to the injected event_publisher_, so events
never leave the process; update Context::publish to, after assembling the
PublishEvent (event) and before returning, check that event_publisher_ is set
and call its publish method (or emit via the publisher API) with the
PublishEvent or JSON payload (use existing nlohmann::json pub_data if the
publisher expects JSON), and ensure errors are handled/logged; ensure
Context::finalize still calls publish() and returns the publisher-forwarded
result (or the same PublishEvent) so the external DefaultContextEventPublisher
actually receives events.
- Around line 90-103: The non-blocking readiness check in Context::is_ready is
broken because it treats deferred futures as not-ready; either make
Client::get_context_data produce an eager future by switching its std::async to
use std::launch::async, or modify Context::is_ready to detect
std::future_status::deferred and explicitly trigger or poll the underlying HTTP
future (the future returned by Client::get_context_data) so that become_ready()
is called when work completes; update the code paths consistently
(Client::get_context_data and Context::is_ready) to choose one approach and
ensure deferred futures no longer cause is_ready() to always return false.
In `@src/default_context_data_provider.cpp`:
- Around line 5-6: The constructor
DefaultContextDataProvider::DefaultContextDataProvider should reject null
shared_ptrs to avoid later dereference in get_context_data(): validate the
incoming std::shared_ptr<Client> parameter (client) and if it is null throw a
std::invalid_argument or std::runtime_error (with a clear message) or use
assert, then move it into client_; this ensures client_ is never null when
methods like get_context_data() access it.
In `@src/default_context_event_publisher.cpp`:
- Around line 5-6: The constructor
DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client>
client) currently accepts a null client_ and later dereferences it; change the
constructor to validate the incoming shared_ptr and fail fast by rejecting null
(e.g., throw std::invalid_argument or use assert) if client is null, so client_
is guaranteed non-null for subsequent use. Ensure the check occurs before
assigning or using client_ and include a clear error message referencing the
Client parameter.
In `@src/default_http_client.cpp`:
- Around line 15-105: The async lambdas in DefaultHTTPClient::get, ::put and
::post capture raw this and call the member perform_request, risking
use-after-free if the client is destroyed before the future completes; make
DefaultHTTPClient inherit std::enable_shared_from_this<DefaultHTTPClient> (or
otherwise obtain a shared_ptr to the instance), then replace the capture of raw
this with a captured shared_ptr (or capture a weak_ptr and lock it inside the
lambda) so the object is kept alive while the async task runs; update the
lambdas in get/put/post to capture e.g. self = shared_from_this() (or weak_self
and lock to call perform_request) and use self->perform_request(...) instead of
calling perform_request() on a raw this.
- Around line 145-152: In DefaultHTTPClient::perform_request, set libcurl
connection and overall timeouts via curl_easy_setopt before calling
curl_easy_perform to prevent indefinite blocks: call curl_easy_setopt(curl,
CURLOPT_CONNECTTIMEOUT_MS, ...) and curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS,
...) (or the _SECONDS variants) using sensible defaults or configurable values,
ensuring these options are applied alongside the existing
CURLOPT_WRITEFUNCTION/WRITEDATA setup so async worker threads cannot hang on
DNS/half-open connections.
- Around line 7-13: Remove curl_global_init()/curl_global_cleanup() from
DefaultHTTPClient::DefaultHTTPClient and DefaultHTTPClient::~DefaultHTTPClient
and make global init/cleanup happen exactly once for the process: implement a
helper like ensureCurlGlobalInit() that uses a static std::once_flag and
std::call_once to call curl_global_init(CURL_GLOBAL_DEFAULT) and registers
curl_global_cleanup() via std::atexit (or equivalent process-lifetime cleanup),
call ensureCurlGlobalInit() where DefaultHTTPClient instances are created (or at
start of DefaultHTTPClient ctor) so initialization is thread-safe and happens
only once, and leave cleanup to the atexit-registered function.
---
Duplicate comments:
In `@src/context.cpp`:
- Around line 202-209: set_override currently mutates overrides_ after
finalization; to match set_custom_assignment behavior, call
check_not_finalized() at the start of set_override before updating overrides_,
and also call check_not_finalized() at the start of set_overrides (or rely on
the per-item set_override check but prefer a single pre-check) to avoid partial
application; reference functions: set_override, set_overrides,
set_custom_assignment and member overrides_.
---
Nitpick comments:
In `@include/absmartly/default_http_client.h`:
- Around line 31-36: Extract the duplicated cURL helper functions
build_url_with_query, build_header_list and write_callback out of the two client
headers and into a single internal utility (e.g., an internal namespace/header +
source file); remove the per-header static duplicates, put shared declarations
in the new header and their implementations in the new .cpp, adjust both
DefaultHttpClient::perform_request and AsyncHttpClient usages to call the shared
functions (or qualify them with the internal namespace), and ensure
build_header_list returns/cleans a curl_slist and write_callback has the same
signature and linkage used by perform_request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c48dd4a9-a6a4-4af0-a639-10b5f0e7e68c
📒 Files selected for processing (21)
CMakeLists.txtinclude/absmartly/async_http_client.hinclude/absmartly/client.hinclude/absmartly/client_config.hinclude/absmartly/context.hinclude/absmartly/context_data_provider.hinclude/absmartly/context_event_publisher.hinclude/absmartly/default_context_data_provider.hinclude/absmartly/default_context_event_publisher.hinclude/absmartly/default_http_client.hinclude/absmartly/http_client.hinclude/absmartly/http_mode.hinclude/absmartly/sdk.hinclude/absmartly/sdk_config.hsrc/async_http_client.cppsrc/client.cppsrc/context.cppsrc/default_context_data_provider.cppsrc/default_context_event_publisher.cppsrc/default_http_client.cppsrc/sdk.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/absmartly/context.h
| find_package(CURL REQUIRED) | ||
| target_sources(absmartly-sdk PRIVATE src/default_http_client.cpp src/async_http_client.cpp) | ||
| target_link_libraries(absmartly-sdk PRIVATE CURL::libcurl) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name CMakeLists.txt | head -5Repository: absmartly/cpp-sdk
Length of output: 100
🏁 Script executed:
cat -n CMakeLists.txt | head -50Repository: absmartly/cpp-sdk
Length of output: 1534
Change CURL linkage to PUBLIC for static library consumers.
absmartly-sdk is created without an explicit library type, defaulting to static. Linking CURL::libcurl as PRIVATE prevents the dependency from propagating to consumers; downstream links will fail with unresolved curl symbols.
🔧 Proposed fix
find_package(CURL REQUIRED)
target_sources(absmartly-sdk PRIVATE src/default_http_client.cpp src/async_http_client.cpp)
-target_link_libraries(absmartly-sdk PRIVATE CURL::libcurl)
+target_link_libraries(absmartly-sdk PUBLIC CURL::libcurl)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_package(CURL REQUIRED) | |
| target_sources(absmartly-sdk PRIVATE src/default_http_client.cpp src/async_http_client.cpp) | |
| target_link_libraries(absmartly-sdk PRIVATE CURL::libcurl) | |
| find_package(CURL REQUIRED) | |
| target_sources(absmartly-sdk PRIVATE src/default_http_client.cpp src/async_http_client.cpp) | |
| target_link_libraries(absmartly-sdk PUBLIC CURL::libcurl) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 37 - 39, The CMake target_link_libraries call
links CURL::libcurl as PRIVATE for target absmartly-sdk; change that linkage to
PUBLIC so the curl dependency propagates to consumers (i.e., replace the PRIVATE
specifier with PUBLIC in the target_link_libraries(absmartly-sdk ...
CURL::libcurl) invocation while keeping find_package(CURL REQUIRED) and the
existing target_sources calls intact).
| explicit DefaultContextDataProvider(std::shared_ptr<Client> client); | ||
|
|
||
| std::future<ContextData> get_context_data() override; | ||
|
|
||
| private: | ||
| std::shared_ptr<Client> client_; |
There was a problem hiding this comment.
Reject null Client instances here as well.
src/default_context_data_provider.cpp Lines 7-9 call client_->get_context_data() unconditionally. As written, DefaultContextDataProvider(nullptr) is accepted and then fails later with a null dereference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/absmartly/default_context_data_provider.h` around lines 11 - 16, The
DefaultContextDataProvider constructor currently accepts a null
std::shared_ptr<Client>, which later causes a null dereference in
get_context_data; update the constructor
(DefaultContextDataProvider(std::shared_ptr<Client> client)) to validate that
client is not null and reject it (e.g., throw std::invalid_argument or use an
assert/Expects) and store the validated pointer in client_; ensure the check is
performed in the constructor implementation so client_->get_context_data() in
get_context_data() is safe.
| explicit DefaultContextEventPublisher(std::shared_ptr<Client> client); | ||
|
|
||
| std::future<void> publish(const PublishEvent& event) override; | ||
|
|
||
| private: | ||
| std::shared_ptr<Client> client_; |
There was a problem hiding this comment.
Reject null Client instances at construction.
src/default_context_event_publisher.cpp Lines 7-9 dereference client_ unconditionally, so DefaultContextEventPublisher(nullptr) compiles and then crashes on the first publish. Validate the pointer in the constructor, or change the type so null is not representable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/absmartly/default_context_event_publisher.h` around lines 11 - 16,
The constructor for DefaultContextEventPublisher must reject null Client
instances because publish() dereferences client_ unconditionally; update the
constructor (DefaultContextEventPublisher) to validate the passed
std::shared_ptr<Client> and throw (e.g., std::invalid_argument) if it's null, or
change the member/type to a non-nullable alternative (e.g., store a Client& or
use a not-null wrapper) so client_ can never be null; ensure the change prevents
construction with nullptr and keeps publish(const PublishEvent&) working without
additional null checks.
| AsyncHTTPClient::AsyncHTTPClient() { | ||
| curl_global_init(CURL_GLOBAL_DEFAULT); | ||
| multi_ = curl_multi_init(); | ||
| worker_ = std::thread(&AsyncHTTPClient::event_loop, this); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "async_http_client.cpp" -o -name "async_http_client.h" | head -20Repository: absmartly/cpp-sdk
Length of output: 128
🏁 Script executed:
git ls-files | grep -E "async_http_client\.(cpp|h)"Repository: absmartly/cpp-sdk
Length of output: 124
🏁 Script executed:
cat -n src/async_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 9428
🏁 Script executed:
cat -n include/absmartly/async_http_client.hRepository: absmartly/cpp-sdk
Length of output: 2827
🌐 Web query:
libcurl curl_global_init return value failure handling
💡 Result:
curl_global_init() returns a CURLcode: CURLE_OK (0) on success; non‑zero on failure. If it fails, do not call any other libcurl functions (treat libcurl as unusable in that process) and return/abort initialization of whatever component depends on libcurl. [1]
Typical handling pattern:
`#include` <curl/curl.h>
`#include` <stdio.h>
int init_http(void) {
CURLcode rc = curl_global_init(CURL_GLOBAL_ALL);
if (rc != CURLE_OK) {
fprintf(stderr, "curl_global_init failed: %s\n", curl_easy_strerror(rc));
return (int)rc; // or map to your app's error codes
}
return 0;
}
void shutdown_http(void) {
curl_global_cleanup();
}Notes that affect “failure handling” in real programs:
- Call it exactly once per process, before other libcurl calls, and pair it with
curl_global_cleanup()on shutdown. [1] - In multi-threaded programs, don’t rely on libcurl’s “lazy” auto-init (triggered by
curl_easy_init()if you didn’t callcurl_global_init()); explicitly initialize early to avoid races because global init may not be thread-safe on some builds/platforms. [2][3] - If you get
CURLE_FAILED_INIT, it generally indicates libcurl couldn’t initialize what it needs (sometimes due to a feature/backend not being available in that build); the error-code list documentsCURLE_FAILED_INITand otherCURLcodevalues. [4]
Sources: curl_global_init man page (return value + “cannot use other curl functions”) [1], thread-safety/lazy-init guidance [2][3], libcurl error codes list [4].
🌐 Web query:
libcurl curl_multi_init return value NULL failure
💡 Result:
curl_multi_init() returns a CURLM * on success and NULL on failure—and if it’s NULL, you must not call other multi-* APIs with it. [1]
What a NULL usually means in practice (C/C++):
- Global init failed / never done: Call
curl_global_init()once (early), and verify it returns 0 before using any libcurl API; if it returns non‑zero “you cannot use the other curl functions.” [2] Also notecurl_global_init()has thread-safety constraints (call it before starting threads). [3] - Out of memory / allocator failure:
curl_multi_init()allocates internal state;NULLcan simply mean allocation failed (no detailed error code is provided by this API). [1]
Minimal defensive pattern:
if (curl_global_init(CURL_GLOBAL_DEFAULT) != 0) {
/* fatal: libcurl not usable */
}
CURLM *mh = curl_multi_init();
if (!mh) {
/* fatal: likely OOM or init problem; do not use mh */
}Sources: Debian/libcurl manpage for curl_multi_init() return value [1]; curl_global_init() return semantics [2]; curl author guidance on explicitly calling curl_global_init() and its thread-safety caveat [3].
[1] https://manpages.debian.org/experimental/libcurl4-doc/curl_multi_init.3.en.html
[2] https://manuals.plus/m/df4904519d18b207dfc7ce12b86b7a747fa122ede7ee95fc024c029487ed4381.pdf
[3] https://daniel.haxx.se/blog/2021/09/27/common-mistakes-when-using-libcurl/
Check return values from libcurl initialisation functions; throw before spawning worker thread.
curl_global_init() and curl_multi_init() both have failure paths. The constructor currently ignores both return values and unconditionally starts the worker_ thread, which will crash on the first curl_multi_add_handle() call if multi_ is null. Check the return code of curl_global_init() and the result of curl_multi_init(); throw an exception before spawning the thread, and unwind the global initialisation if the multi handle fails.
Suggested fix
AsyncHTTPClient::AsyncHTTPClient() {
- curl_global_init(CURL_GLOBAL_DEFAULT);
- multi_ = curl_multi_init();
+ const auto rc = curl_global_init(CURL_GLOBAL_DEFAULT);
+ if (rc != CURLE_OK) {
+ throw std::runtime_error(curl_easy_strerror(rc));
+ }
+ multi_ = curl_multi_init();
+ if (!multi_) {
+ curl_global_cleanup();
+ throw std::runtime_error("Failed to initialise CURL multi handle");
+ }
worker_ = std::thread(&AsyncHTTPClient::event_loop, this);
}Add #include <stdexcept> at the top of this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/async_http_client.cpp` around lines 7 - 10, The AsyncHTTPClient
constructor must check libcurl init return values and throw before starting
worker_; call curl_global_init and verify its return code is CURLE_OK, then call
curl_multi_init and ensure the returned multi_ is non-null; if curl_multi_init
fails unwind by calling curl_global_cleanup() and throw a std::runtime_error
(include <stdexcept> at top) so the worker_ thread is not spawned, and only
start worker_ = std::thread(&AsyncHTTPClient::event_loop, this) after both inits
succeed.
| AsyncHTTPClient::~AsyncHTTPClient() { | ||
| running_ = false; | ||
| pending_cv_.notify_one(); | ||
| if (worker_.joinable()) { | ||
| worker_.join(); | ||
| } | ||
|
|
||
| for (auto& [easy, req] : active_requests_) { | ||
| curl_multi_remove_handle(multi_, easy); | ||
| if (req->headers) { | ||
| curl_slist_free_all(req->headers); | ||
| } | ||
| curl_easy_cleanup(easy); | ||
| req->promise.set_value({0, "Client shutting down", "", {}}); | ||
| delete req; | ||
| } | ||
|
|
||
| curl_multi_cleanup(multi_); | ||
| curl_global_cleanup(); |
There was a problem hiding this comment.
Cancel queued requests as well as active ones during shutdown.
The destructor only walks active_requests_. Any PendingRequest still sitting in pending_queue_ when running_ flips to false is leaked and its future never completes, so callers can hang indefinitely during teardown.
Possible fix
AsyncHTTPClient::~AsyncHTTPClient() {
running_ = false;
pending_cv_.notify_one();
if (worker_.joinable()) {
worker_.join();
}
+
+ {
+ std::lock_guard<std::mutex> lock(pending_mutex_);
+ for (auto* req : pending_queue_) {
+ if (req->headers) {
+ curl_slist_free_all(req->headers);
+ }
+ curl_easy_cleanup(req->easy);
+ req->promise.set_value({0, "Client shutting down", "", {}});
+ delete req;
+ }
+ pending_queue_.clear();
+ }
for (auto& [easy, req] : active_requests_) {
curl_multi_remove_handle(multi_, easy);
if (req->headers) {
curl_slist_free_all(req->headers);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/async_http_client.cpp` around lines 13 - 31, Destructor currently only
cleans active_requests_ leaving PendingRequest objects in pending_queue_ leaked
and their promises unresolved; update AsyncHTTPClient::~AsyncHTTPClient to drain
pending_queue_ during shutdown: after setting running_ = false and notifying
pending_cv_, acquire the pending queue mutex (pending_mutex_), iterate/popleft
each PendingRequest in pending_queue_, free any req->headers
(curl_slist_free_all), set the req->promise with a cancellation result (e.g.,
status 0 and "Client shutting down"), delete the PendingRequest, and clear the
queue before proceeding to clean active_requests_ and join; this ensures all
pending futures are completed and no leaks remain.
| DefaultContextDataProvider::DefaultContextDataProvider(std::shared_ptr<Client> client) | ||
| : client_(std::move(client)) {} |
There was a problem hiding this comment.
Reject null clients in the constructor.
Line 9 dereferences client_ unconditionally, so this wrapper should validate its std::shared_ptr<Client> up front rather than crash when get_context_data() is called.
Proposed fix
+#include <stdexcept>
+
namespace absmartly {
DefaultContextDataProvider::DefaultContextDataProvider(std::shared_ptr<Client> client)
- : client_(std::move(client)) {}
+ : client_(std::move(client)) {
+ if (!client_) {
+ throw std::invalid_argument("client must not be null");
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/default_context_data_provider.cpp` around lines 5 - 6, The constructor
DefaultContextDataProvider::DefaultContextDataProvider should reject null
shared_ptrs to avoid later dereference in get_context_data(): validate the
incoming std::shared_ptr<Client> parameter (client) and if it is null throw a
std::invalid_argument or std::runtime_error (with a clear message) or use
assert, then move it into client_; this ensures client_ is never null when
methods like get_context_data() access it.
| DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client> client) | ||
| : client_(std::move(client)) {} |
There was a problem hiding this comment.
Reject null clients in the constructor.
Line 9 dereferences client_ unconditionally, so accepting a null std::shared_ptr<Client> here turns a configuration mistake into a later crash. Fail fast in the constructor instead.
Proposed fix
+#include <stdexcept>
+
namespace absmartly {
DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client> client)
- : client_(std::move(client)) {}
+ : client_(std::move(client)) {
+ if (!client_) {
+ throw std::invalid_argument("client must not be null");
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client> client) | |
| : client_(std::move(client)) {} | |
| DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client> client) | |
| : client_(std::move(client)) { | |
| if (!client_) { | |
| throw std::invalid_argument("client must not be null"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/default_context_event_publisher.cpp` around lines 5 - 6, The constructor
DefaultContextEventPublisher::DefaultContextEventPublisher(std::shared_ptr<Client>
client) currently accepts a null client_ and later dereferences it; change the
constructor to validate the incoming shared_ptr and fail fast by rejecting null
(e.g., throw std::invalid_argument or use assert) if client is null, so client_
is guaranteed non-null for subsequent use. Ensure the check occurs before
assigning or using client_ and include a clear error message referencing the
Client parameter.
| DefaultHTTPClient::DefaultHTTPClient() { | ||
| curl_global_init(CURL_GLOBAL_DEFAULT); | ||
| } | ||
|
|
||
| DefaultHTTPClient::~DefaultHTTPClient() { | ||
| curl_global_cleanup(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.cpp" -o -name "*.h" | grep -E "(http_client|curl)" | head -20Repository: absmartly/cpp-sdk
Length of output: 234
🏁 Script executed:
git ls-files | grep -E "(default_http_client|http_client)" | head -20Repository: absmartly/cpp-sdk
Length of output: 224
🏁 Script executed:
cat -n src/default_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 7838
🏁 Script executed:
cat -n src/default_http_client.hRepository: absmartly/cpp-sdk
Length of output: 118
🏁 Script executed:
rg "DefaultHTTPClient" --type cpp --type hRepository: absmartly/cpp-sdk
Length of output: 1497
🏁 Script executed:
cat -n include/absmartly/default_http_client.hRepository: absmartly/cpp-sdk
Length of output: 1910
🏁 Script executed:
cat -n src/sdk.cpp | head -100Repository: absmartly/cpp-sdk
Length of output: 3174
🏁 Script executed:
rg "make_shared<DefaultHTTPClient>" --type cpp --type hRepository: absmartly/cpp-sdk
Length of output: 133
🏁 Script executed:
web_search
libcurl curl_global_init curl_global_cleanup process global initialization requirementsRepository: absmartly/cpp-sdk
Length of output: 157
🌐 Web query:
libcurl curl_global_init curl_global_cleanup process global initialization requirements
💡 Result:
- Scope:
curl_global_init()/curl_global_cleanup()are process-wide (apply to “all the code that shares a memory space”), not per easy handle. (ocgis.com) - When to call init: Call
curl_global_init(CURL_GLOBAL_ALL)exactly once early in program startup, before any other libcurl function is used (and ideally before you start other threads). (curl.se) - Thread-safety of init:
curl_global_init()is not thread-safe, so you must ensure it is not called concurrently and is completed before other threads that might touch libcurl (or related libs) run. (everything.curl.dev) - Matching cleanup: Call
curl_global_cleanup()once for each successfulcurl_global_init(), after you are completely done with libcurl. (curl.se) - Thread-safety of cleanup: Do not call
curl_global_cleanup()while any other thread in the same process is running (so, practically: call it during single-threaded shutdown, after all libcurl work has stopped). (curl.se) - If you skip init: libcurl may perform lazy “fail-safe” initialization on first use (e.g., via
curl_easy_init()), but in multi-threaded programs you should not rely on this—explicitly callcurl_global_init()yourself. (everything.curl.dev) - Special case (SSL backend selection): If you need to choose an SSL backend in multi-backend builds, call
curl_global_sslset()beforecurl_global_init(). (curl.se)
Citations:
- 1: https://www.ocgis.com/documents/OCSurvey/MDC/MDC2KML/app/cURL/Manual/libcurl/curl_global_init.html?utm_source=openai
- 2: https://curl.se/libcurl/c/libcurl.html?utm_source=openai
- 3: https://everything.curl.dev/libcurl/globalinit.html?utm_source=openai
- 4: https://curl.se/libcurl/c/curl_global_cleanup.html?utm_source=openai
- 5: https://curl.se/libcurl/c/curl_global_cleanup.html?utm_source=openai
- 6: https://everything.curl.dev/libcurl/globalinit.html?utm_source=openai
- 7: https://curl.se/libcurl/c/libcurl.html?utm_source=openai
Move curl_global_init() / curl_global_cleanup() out of object lifetime.
libcurl requires curl_global_init() to be called exactly once early in program startup, before any other libcurl calls, and in a thread-safe manner. Similarly, curl_global_cleanup() must not be called whilst other threads are using libcurl. Tying these to individual DefaultHTTPClient instances violates this contract: if multiple clients are created, curl_global_init() is called multiple times; when one client is destroyed, curl_global_cleanup() is called prematurely whilst async lambdas may still be executing in other threads.
🔧 Proposed fix
+#include <mutex>
+
+namespace {
+std::once_flag curl_init_once;
+}
+
DefaultHTTPClient::DefaultHTTPClient() {
- curl_global_init(CURL_GLOBAL_DEFAULT);
+ std::call_once(curl_init_once, [] {
+ curl_global_init(CURL_GLOBAL_DEFAULT);
+ });
}
-DefaultHTTPClient::~DefaultHTTPClient() {
- curl_global_cleanup();
-}
+DefaultHTTPClient::~DefaultHTTPClient() = default;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DefaultHTTPClient::DefaultHTTPClient() { | |
| curl_global_init(CURL_GLOBAL_DEFAULT); | |
| } | |
| DefaultHTTPClient::~DefaultHTTPClient() { | |
| curl_global_cleanup(); | |
| } | |
| `#include` <mutex> | |
| namespace { | |
| std::once_flag curl_init_once; | |
| } | |
| DefaultHTTPClient::DefaultHTTPClient() { | |
| std::call_once(curl_init_once, [] { | |
| curl_global_init(CURL_GLOBAL_DEFAULT); | |
| }); | |
| } | |
| DefaultHTTPClient::~DefaultHTTPClient() = default; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/default_http_client.cpp` around lines 7 - 13, Remove
curl_global_init()/curl_global_cleanup() from
DefaultHTTPClient::DefaultHTTPClient and DefaultHTTPClient::~DefaultHTTPClient
and make global init/cleanup happen exactly once for the process: implement a
helper like ensureCurlGlobalInit() that uses a static std::once_flag and
std::call_once to call curl_global_init(CURL_GLOBAL_DEFAULT) and registers
curl_global_cleanup() via std::atexit (or equivalent process-lifetime cleanup),
call ensureCurlGlobalInit() where DefaultHTTPClient instances are created (or at
start of DefaultHTTPClient ctor) so initialization is thread-safe and happens
only once, and leave cleanup to the atexit-registered function.
| std::future<HTTPClient::Response> DefaultHTTPClient::get(const std::string& url, | ||
| const std::map<std::string, std::string>& query, | ||
| const std::map<std::string, std::string>& headers) { | ||
| return std::async(std::launch::async, [this, url, query, headers]() -> HTTPClient::Response { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) { | ||
| return {500, "Failed to initialize CURL", "", {}}; | ||
| } | ||
|
|
||
| std::string full_url = build_url_with_query(url, query); | ||
| curl_easy_setopt(curl, CURLOPT_URL, full_url.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L); | ||
|
|
||
| struct curl_slist* header_list = build_header_list(headers); | ||
| if (header_list) { | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, header_list); | ||
| } | ||
|
|
||
| auto response = perform_request(curl); | ||
|
|
||
| if (header_list) { | ||
| curl_slist_free_all(header_list); | ||
| } | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return response; | ||
| }); | ||
| } | ||
|
|
||
| std::future<HTTPClient::Response> DefaultHTTPClient::put(const std::string& url, | ||
| const std::map<std::string, std::string>& query, | ||
| const std::map<std::string, std::string>& headers, | ||
| const std::vector<uint8_t>& body) { | ||
| return std::async(std::launch::async, [this, url, query, headers, body]() -> HTTPClient::Response { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) { | ||
| return {500, "Failed to initialize CURL", "", {}}; | ||
| } | ||
|
|
||
| std::string full_url = build_url_with_query(url, query); | ||
| curl_easy_setopt(curl, CURLOPT_URL, full_url.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "PUT"); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDS, body.data()); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, static_cast<long>(body.size())); | ||
|
|
||
| struct curl_slist* header_list = build_header_list(headers); | ||
| if (header_list) { | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, header_list); | ||
| } | ||
|
|
||
| auto response = perform_request(curl); | ||
|
|
||
| if (header_list) { | ||
| curl_slist_free_all(header_list); | ||
| } | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return response; | ||
| }); | ||
| } | ||
|
|
||
| std::future<HTTPClient::Response> DefaultHTTPClient::post(const std::string& url, | ||
| const std::map<std::string, std::string>& query, | ||
| const std::map<std::string, std::string>& headers, | ||
| const std::vector<uint8_t>& body) { | ||
| return std::async(std::launch::async, [this, url, query, headers, body]() -> HTTPClient::Response { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) { | ||
| return {500, "Failed to initialize CURL", "", {}}; | ||
| } | ||
|
|
||
| std::string full_url = build_url_with_query(url, query); | ||
| curl_easy_setopt(curl, CURLOPT_URL, full_url.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_POST, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDS, body.data()); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, static_cast<long>(body.size())); | ||
|
|
||
| struct curl_slist* header_list = build_header_list(headers); | ||
| if (header_list) { | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, header_list); | ||
| } | ||
|
|
||
| auto response = perform_request(curl); | ||
|
|
||
| if (header_list) { | ||
| curl_slist_free_all(header_list); | ||
| } | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return response; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and locate the relevant files
fd -t f "default_http_client" --extension cpp --extension hRepository: absmartly/cpp-sdk
Length of output: 128
🏁 Script executed:
# Read the header file to understand the class definition
head -100 src/default_http_client.hRepository: absmartly/cpp-sdk
Length of output: 145
🏁 Script executed:
# Read the implementation file to understand the full context
cat -n src/default_http_client.cpp | head -150Repository: absmartly/cpp-sdk
Length of output: 6635
🏁 Script executed:
# Read the header file
cat -n include/absmartly/default_http_client.hRepository: absmartly/cpp-sdk
Length of output: 1910
🏁 Script executed:
# Check if there's any usage pattern or tests that show how futures are handled
fd -t f "default_http_client" -o "test" -o "example" | head -20Repository: absmartly/cpp-sdk
Length of output: 188
🏁 Script executed:
# Search for where DefaultHTTPClient is instantiated or used
rg "DefaultHTTPClient" -t cpp -t h | head -30Repository: absmartly/cpp-sdk
Length of output: 1497
Avoid capturing raw this in async request tasks; use std::enable_shared_from_this or capture a shared pointer instead.
Each request method launches std::async(std::launch::async, [this, ...]) and calls perform_request(), which is a member function. If the client instance is destroyed before the returned future completes, the lambda will dereference a dangling pointer when it tries to invoke perform_request(). This is a use-after-free bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/default_http_client.cpp` around lines 15 - 105, The async lambdas in
DefaultHTTPClient::get, ::put and ::post capture raw this and call the member
perform_request, risking use-after-free if the client is destroyed before the
future completes; make DefaultHTTPClient inherit
std::enable_shared_from_this<DefaultHTTPClient> (or otherwise obtain a
shared_ptr to the instance), then replace the capture of raw this with a
captured shared_ptr (or capture a weak_ptr and lock it inside the lambda) so the
object is kept alive while the async task runs; update the lambdas in
get/put/post to capture e.g. self = shared_from_this() (or weak_self and lock to
call perform_request) and use self->perform_request(...) instead of calling
perform_request() on a raw this.
| HTTPClient::Response DefaultHTTPClient::perform_request(CURL* curl) { | ||
| Response response; | ||
| std::vector<uint8_t> response_body; | ||
|
|
||
| curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_callback); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response_body); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "default_http_client.cpp"Repository: absmartly/cpp-sdk
Length of output: 88
🏁 Script executed:
wc -l src/default_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 92
🏁 Script executed:
sed -n '130,165p' src/default_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 1172
🏁 Script executed:
# Search for timeout-related configurations in the codebase
rg "CURLOPT_TIMEOUT|CURLOPT_CONNECTTIMEOUT" --type cppRepository: absmartly/cpp-sdk
Length of output: 43
🏁 Script executed:
# Search for CURL handle initialization and configuration patterns
rg "curl_easy_setopt" src/default_http_client.cpp -B 2 -A 2Repository: absmartly/cpp-sdk
Length of output: 1672
🏁 Script executed:
# Look for global CURL configuration or initialization
rg "perform_request\|curl_easy_perform" src/default_http_client.cpp -B 5 -A 10Repository: absmartly/cpp-sdk
Length of output: 43
🏁 Script executed:
# Find all files that might initialize CURL handles
fd -t f "\.h$|\.hpp$|\.cpp$" | xargs grep -l "curl_easy_new\|CURL\*" | head -20Repository: absmartly/cpp-sdk
Length of output: 192
🏁 Script executed:
# Look at the full perform_request method and surrounding context
sed -n '140,160p' src/default_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 692
🏁 Script executed:
# Search for where perform_request is called from
rg "perform_request" --type cpp -B 3 -A 3Repository: absmartly/cpp-sdk
Length of output: 2289
🏁 Script executed:
# Check the DefaultHTTPClient class definition
rg "class DefaultHTTPClient" -A 20 --type h --type hppRepository: absmartly/cpp-sdk
Length of output: 88
🏁 Script executed:
# Search for DefaultHTTPClient class in header files
rg "class DefaultHTTPClient" -A 20Repository: absmartly/cpp-sdk
Length of output: 1949
🏁 Script executed:
# Look at the complete default_http_client.h file
cat -n include/absmartly/default_http_client.hRepository: absmartly/cpp-sdk
Length of output: 1910
🏁 Script executed:
# Look for curl_easy_new or CURL handle creation
rg "curl_easy_new\|curl_easy_init" --type cpp -B 5 -A 5Repository: absmartly/cpp-sdk
Length of output: 43
🏁 Script executed:
# Check if there's any global configuration or shared CURL setup
rg "curl_global\|DefaultHTTPClient(" --type cpp -B 3 -A 3Repository: absmartly/cpp-sdk
Length of output: 173
🏁 Script executed:
# Search for DefaultHTTPClient constructor
rg "DefaultHTTPClient::" --type cpp -B 2 -A 10 | head -60Repository: absmartly/cpp-sdk
Length of output: 4174
🏁 Script executed:
# Look at the entire perform_request implementation
sed -n '145,165p' src/default_http_client.cppRepository: absmartly/cpp-sdk
Length of output: 804
🏁 Script executed:
# Check if any CURL options are set in async_http_client or elsewhere
cat -n src/async_http_client.cpp | head -80Repository: absmartly/cpp-sdk
Length of output: 3013
Add connection and overall timeouts before calling curl_easy_perform.
Without timeout configuration, DNS lookups or half-open connections can block indefinitely, causing worker threads to stall and SDK calls to hang on returned futures. This is a significant reliability issue given that requests execute in std::async worker threads.
Suggested fix
HTTPClient::Response DefaultHTTPClient::perform_request(CURL* curl) {
Response response;
std::vector<uint8_t> response_body;
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_callback);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response_body);
+ curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT_MS, 5000L);
+ curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS, 30000L);
CURLcode res = curl_easy_perform(curl);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HTTPClient::Response DefaultHTTPClient::perform_request(CURL* curl) { | |
| Response response; | |
| std::vector<uint8_t> response_body; | |
| curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_callback); | |
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response_body); | |
| CURLcode res = curl_easy_perform(curl); | |
| HTTPClient::Response DefaultHTTPClient::perform_request(CURL* curl) { | |
| Response response; | |
| std::vector<uint8_t> response_body; | |
| curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_callback); | |
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response_body); | |
| curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT_MS, 5000L); | |
| curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS, 30000L); | |
| CURLcode res = curl_easy_perform(curl); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/default_http_client.cpp` around lines 145 - 152, In
DefaultHTTPClient::perform_request, set libcurl connection and overall timeouts
via curl_easy_setopt before calling curl_easy_perform to prevent indefinite
blocks: call curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT_MS, ...) and
curl_easy_setopt(curl, CURLOPT_TIMEOUT_MS, ...) (or the _SECONDS variants) using
sensible defaults or configurable values, ensuring these options are applied
alongside the existing CURLOPT_WRITEFUNCTION/WRITEDATA setup so async worker
threads cannot hang on DNS/half-open connections.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
11-37:⚠️ Potential issue | 🟡 MinorFix the heading level jump under
Installation.These subsections still jump from
##straight to####, which violates markdownlint’s heading-increment rule. They should be###.📝 Proposed change
-#### CMake (FetchContent) +### CMake (FetchContent) @@ -#### CMake (Local) +### CMake (Local) @@ -#### Building from Source +### Building from Source🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 11 - 37, The subsections under the Installation section use level-4 headings (e.g., "#### CMake (FetchContent)", "#### CMake (Local)", "#### Building from Source") and should be level-3 to comply with markdownlint heading-increment rules; update those headings in README.md from "####" to "###" so they correctly nest under the "## Installation" heading (locate the lines containing the CMake (FetchContent), CMake (Local), and Building from Source headings and change their leading hashes).
482-496:⚠️ Potential issue | 🟡 MinorUse canonical GitHub URLs in the SDK list.
These links still point to
www.github.com; switching them togithub.comavoids redirect-only URLs in the README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 482 - 496, The SDK links in the README (the bullet list containing entries like "Java SDK", "JavaScript SDK", "C++ SDK" etc.) use redirecting URLs with "www.github.com"; update each link target to the canonical "github.com" domain (e.g., change "https://www.github.com/absmartly/..." to "https://github.com/absmartly/...") so the README uses non-redirecting canonical GitHub URLs.
🧹 Nitpick comments (1)
README.md (1)
18-22: Use an immutable commit hash instead ofGIT_TAG mainin the FetchContent example.Using a moving branch makes downstream builds non-reproducible and can break consumers unexpectedly. CMake's FetchContent documentation explicitly recommends using a commit hash rather than a branch or tag name, for security and reproducibility.
♻️ Proposed change
FetchContent_Declare( absmartly-sdk GIT_REPOSITORY https://github.com/absmartly/cpp-sdk.git - GIT_TAG main + GIT_TAG abc1234def5678 # v1.0.0 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 18 - 22, The FetchContent snippet uses a moving branch ("GIT_TAG main") which should be replaced with an immutable commit hash to ensure reproducible builds; update the FetchContent_Declare block for absmartly-sdk so that GIT_TAG references a specific commit SHA (replace "main" with a full commit hash) or use GIT_TAG/URL fields to pin to that commit, and commit the updated README text so the example shows the fixed commit hash instead of "main".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 65-71: The README quick-start uses C++20 designated initializers
(e.g., .endpoint = "...") while the project claims C++17 compatibility; update
the examples to either (A) use C++17-compatible initialization by creating a
config object and assigning fields before passing it to
absmartly::SDK::create_simple (e.g., Config cfg; cfg.endpoint = "...";
cfg.api_key = "..."; absmartly::SDK::create_simple(cfg);), or (B) change the
documented minimum standard to C++20. Make this change for all occurrences (the
examples that call absmartly::SDK::create_simple) so the README compiles under
the stated standard.
---
Duplicate comments:
In `@README.md`:
- Around line 11-37: The subsections under the Installation section use level-4
headings (e.g., "#### CMake (FetchContent)", "#### CMake (Local)", "####
Building from Source") and should be level-3 to comply with markdownlint
heading-increment rules; update those headings in README.md from "####" to "###"
so they correctly nest under the "## Installation" heading (locate the lines
containing the CMake (FetchContent), CMake (Local), and Building from Source
headings and change their leading hashes).
- Around line 482-496: The SDK links in the README (the bullet list containing
entries like "Java SDK", "JavaScript SDK", "C++ SDK" etc.) use redirecting URLs
with "www.github.com"; update each link target to the canonical "github.com"
domain (e.g., change "https://www.github.com/absmartly/..." to
"https://github.com/absmartly/...") so the README uses non-redirecting canonical
GitHub URLs.
---
Nitpick comments:
In `@README.md`:
- Around line 18-22: The FetchContent snippet uses a moving branch ("GIT_TAG
main") which should be replaced with an immutable commit hash to ensure
reproducible builds; update the FetchContent_Declare block for absmartly-sdk so
that GIT_TAG references a specific commit SHA (replace "main" with a full commit
hash) or use GIT_TAG/URL fields to pin to that commit, and commit the updated
README text so the example shows the fixed commit hash instead of "main".
| int main() { | ||
| auto sdk = absmartly::SDK::create_simple({ | ||
| .endpoint = "https://your-company.absmartly.io/v1", | ||
| .api_key = "YOUR_API_KEY", | ||
| .application = "website", | ||
| .environment = "production" | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does C++17 support designated initialisers like .endpoint = "..." for object initialisation? Please include GCC, Clang, and MSVC support notes.
💡 Result:
No. C++17 (the standard) does not have designated initializers of the form .endpoint = "..."; designated initializers were standardized in C++20 (P0329R4). Using them in C++17 is therefore non-portable and, at best, relies on compiler extensions. [1]
GCC
- In
-std=c++17/-std=gnu++17, GCC has historically accepted C99/GNU-style designated-initializer syntax as an extension in many cases (often with warnings depending on flags), but it’s not C++17-standard code. [2] - For standard C++ designated initializers, cppreference lists support as GCC 8 (with earlier partial support noted). [1]
Clang
- In
-std=c++17, Clang also accepts designated initializers as an extension (originally via its C99-designator support), again making it non-standard in C++17. [3] - For standard C++20 designated initializers, Clang’s C++ status page lists support starting in Clang 10. [4] (cppreference also lists Clang support accordingly). [1]
MSVC
- MSVC does not generally accept
.member = ...in C++17 mode; you typically need/std:c++20or/std:c++latestfor designated initialization of members. [5] - cppreference lists C++20 designated initializer support starting at MSVC 19.21. [1]
Bottom line: if you need .endpoint = "..." and want portability, treat it as C++20+, not C++17. [1]
Sources
[1] cppreference – C++ compiler support (Designated initializers / P0329R4) (en.cppreference.com)
[2] Stack Overflow – designated initializers in C++17 and GCC extension behavior (stackoverflow.com)
[3] Stack Overflow – Clang allowing designated initializers in C++ as an extension (stackoverflow.com)
[4] Clang – C++ support (cxx_status) (Designated initializers: Clang 10) (clang.llvm.org)
[5] Microsoft Q&A – designated initialization works with /std:c++20 (learn.microsoft.com)
Citations:
- 1: https://en.cppreference.com/w/cpp/compiler_support?trk=public_post_comment-text
- 2: https://stackoverflow.com/questions/69009389/how-to-get-away-with-using-designated-initializers-in-c17-or-why-is-it-seemi?utm_source=openai
- 3: https://stackoverflow.com/questions/54459635/why-has-clang-decided-to-allow-designated-initializers-in-c?utm_source=openai
- 4: https://clang.llvm.org/cxx_status?utm_source=openai
- 5: https://learn.microsoft.com/en-us/answers/questions/1165265/c-array-designator-initializer-does-not-work-with?utm_source=openai
The quick-start examples use C++20 designated initialisers against a C++17 compatibility claim.
Lines 65–71 and 205–210 rely on designated initialiser syntax (.endpoint = ...), which is not supported in C++17. Designated initialisers were introduced in C++20 (P0329R4). MSVC does not support this syntax in C++17 mode and will fail to compile these examples; GCC and Clang provide only non-standard extension support.
Either rewrite the examples using standard C++17 syntax (e.g., named config objects with assignment), or update the documented minimum standard to C++20.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 65 - 71, The README quick-start uses C++20 designated
initializers (e.g., .endpoint = "...") while the project claims C++17
compatibility; update the examples to either (A) use C++17-compatible
initialization by creating a config object and assigning fields before passing
it to absmartly::SDK::create_simple (e.g., Config cfg; cfg.endpoint = "...";
cfg.api_key = "..."; absmartly::SDK::create_simple(cfg);), or (B) change the
documented minimum standard to C++20. Make this change for all occurrences (the
examples that call absmartly::SDK::create_simple) so the README compiles under
the stated standard.
1 participant
Summary
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests