feat(pj_base): centralize string→number parsing via fast_float private dep#101
Merged
Conversation
…e dep
Replace the floating-point branch of PJ::parseNumber<T> with a fast_float
implementation, kept in a new .cpp so the dependency is private and never
visible in installed headers or exported CMake targets. Integer branch
continues to use std::from_chars in the header.
Why fast_float matters:
- Locale-independent. std::strto* respects LC_NUMERIC, so a user under a
de_DE locale silently parsed "1.5" as 1. This was a latent correctness
bug affecting any English-formatted telemetry consumed under non-C
locales.
- Zero allocation. No copy of the string_view to a null-terminated buffer.
- Drops the Apple Clang libc++ <charconv> floating-point workaround that
the previous implementation needed.
Privacy guarantee verified:
- fast_float is declared in conanfile.py with visible=False and
transitive_*=False (mirrors the existing fmt pattern).
- pj_base links FastFloat::fast_float under $<BUILD_INTERFACE:...> so it
is stripped from the installed plotjuggler_coreTargets file.
- An install-tree grep confirms no fast_float #include and no FastFloat
reference reaches downstream consumers.
Tests:
- New pj_base/tests/number_parse_test.cpp covering int success/failure,
float success/failure, a de_DE-locale regression guard, and the
long double strtold fallback path. 62/62 tests pass.
In-tree migrations (13 sites):
- pj_plugins/dialog_protocol/widget_data_view.hpp — three std::stoi
parses of row/col indices.
- pj_plugins/examples/mock_{json,schema}_parser.cpp — three std::strtod
calls. The new code returns a proper parse error instead of silently
producing 0 on garbage input.
- pj_datastore/examples/parquet_import.cpp — std::atoi for chunk_rows;
now prints an error and exits 1 on invalid input (the previous
garbage→0 behavior was a latent bug).
Version bump:
- 0.4.1 → 0.4.2 (PATCH: public template signature unchanged).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pabloinigoblasco
approved these changes
May 28, 2026
Collaborator
pabloinigoblasco
left a comment
pabloinigoblasco
left a comment
There was a problem hiding this comment.
Unifying style: good!
Code looks correct.
I approve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PJ::parseNumber<T>with afast_float-backed implementation in a newpj_base/src/number_parse.cpp. fast_float is added as a private Conan dep (visible=False, transitive_*=False, mirrors the existingfmtpattern) and linked under$<BUILD_INTERFACE:FastFloat::fast_float>so it never appears in installed headers or the exportedplotjuggler_coreTargets*.cmake. Integer branch keepsstd::from_charsin the header.std::strto*respectedLC_NUMERICand silently parsed"1.5"as 1 under ade_DElocale. Also drops the per-callstd::stringallocation that the previous null-terminatedstrto*path needed.0.4.1 → 0.4.2.Three in-tree call-sites migrated alongside the refactor: three
std::stoiinwidget_data_view.hpp, threestd::strtodin the mock parsers (mock_json_parser.cpp+mock_schema_parser.cpp), andstd::atoiin theparquet_importexample (changed to error+exit on invalid input — the previous garbage→0 was a latent bug).Test plan
-Wall -Wextra -Werrornumber_parse_testwith 6 cases covering int success/failure, float success/failure, de_DE locale regression guard, andlong doublestrtold fallbackcmake --installto a temp prefix, then grep — nofast_float#includein the installed include tree, noFastFloatreference inlib/cmake/plotjuggler_core/./build.sh --debug) — pre-existing environment issue with ncurses/gcc-15 (transitive of cpython/arrow) blocked the local Debug run; CI should validateFollow-ups
feat/parser-ros-frametransformsit stacks on has diverged from origin).pj-official-pluginsrepo has 15 cleanly-replaceable call sites across 7 plugins; survey roadmap is ready and will land as per-plugin PRs onceplotjuggler_core/0.4.2is on the Cloudsmith remote.🤖 Generated with Claude Code