fmtlib adoption extension#3375
Draft
Lestropie wants to merge 9 commits into
Draft
Conversation
Delete the str<cfloat>/str<cdouble> specialisations in core/mrtrix.h, which
emitted a bespoke complex representation (re+imi, parens-free, imaginary part
suppressed when zero). All complex str() call sites now route through fmtlib's
<fmt/std.h> formatter ((re+imi) form) via explicit fmt::format("{}", ...):
- core/stats.h: mean/std/std_rv/min/max (refined further in Task 2)
- cmd/mrcalc.cpp: calculator-mode constant stringification
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the runtime is_complex flag in MR::Stats with compile-time type
dispatch, so guaranteed-real data never engages fmtlib's complex formatter:
- Stats becomes a class template Stats<T> (T in {default_type, cdouble}); the
Welford accumulators are typed T, with if constexpr (is_complex<T>) selecting
the real/imag-separated min/max/std logic vs the scalar form. std_rv and the
median values vector are real-valued and guarded accordingly. operator() is
explicitly instantiated for both domains; print_header is now a template.
- mrstats reads header.datatype().is_complex() once and dispatches to a
templated run_impl<T>() opening Image<T>; Volume_loop and run_volume are
templated on T.
Adopting fmtlib's (a+bi) complex form left MR::to<cfloat>/to<cdouble> unable to
parse the software's own output (still only read the bare a+bi). Extend both
parsers to also accept the parenthesised form, restoring write/read symmetry
(mrcalc complex constants, complex matrix I/O, the diff-matrix test harness).
Extend the MR::to<>() unit test (to_tests.cpp) with parenthesised inputs and a
round-trip value check. The mrstats complex reference test gains -frac 1e-5 to
accommodate fmt's shortest-round-trip precision (matching its real siblings).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Convert the 13 str(value, precision) sites per per-site decisions, replacing
each with fmt::format. Discard the precision (accept fmt's shortest round-trip)
where it was incidental, and propagate it as {:.Ng} where an external spec or a
fixed display format demands a stable digit count:
- Discard: matrix.h vector .txt writes (reload exactly via to<>); MGH header
keyvals (TR/flip/TE/TI); DICOM value-string; mrinfo voxel-size description;
BIDS TotalReadoutTime (default precision).
- Propagate: DICOM time subseconds {:.6g} (DICOM TM allows <=6 fractional
digits); track-file timestamp {:.{}g} with file_timestamp_precision (20);
MRView FPS readout {:.4g}.
Converting the generic matrix.h vector write to fmt exposed Eigen::half, which
has no native fmt formatter; add an fmt::formatter<Eigen::half> : ostream_formatter
bridge in half.h (delegates to Eigen's operator<<). [Recorded for the Task 7
nontrivial-class tabulation.]
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 17 str<T>(expr) coercion sites forced a target type before stringifying.
Replace each with fmt::format("{}", static_cast<T>(expr)): the static_cast
performs the type coercion (for ParsedArgument operands this exercises the
intended operator unsigned int()/operator float() conversion), and fmt's
shortest-round-trip formatting replaces str()'s max_digits10 default.
Stream call sites (5ttedit verbose output, surface/mesh.cpp VTK vertex writes,
tckstats histogram counts) are wrapped in fmt::format rather than emitted bare
into the ostream, so fmt's formatting is retained instead of falling back to
iostream's 6-significant-figure default. Keyval/properties assignments
(mrhistmatch, seeding, tracking, iFOD1/2) take the std::string result directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the bespoke fmt container/Eigen formatters in core/types.h with fmtlib's
native facilities:
- Delete the custom formatter<std::vector<T>> and formatter<std::array<T,N>>;
add <fmt/ranges.h>. Container output style changes from "[ x y z ]" to fmt's
"[1, 2, 3]" (strings quoted).
- The Eigen matrix/array formatter now delegates to Eigen's own ostream operator<<
(via fmt::streamed), special-casing a column vector by emitting its transpose on
a single line with a trailing "^T" (transpose done inside the formatter). Since
Eigen types satisfy fmtlib's is_range, range_format_kind<Eigen...> is specialised
to range_format::disabled so the dedicated formatter applies unambiguously rather
than colliding with the generic range formatter. Eigen::Transform and
Eigen::WithFormat formatters are retained.
Call-site/test rectification for the style change:
- dirstat.cpp: a vector formatted with "{:.6g}" must use fmtlib's per-element range
spec "{::.6g}" (a whole-range numeric spec now throws at runtime).
- dirrotate/labelvalidate tests parsed command output by field position / spaced
tokens; updated to the new comma/bracket layout (strip brackets+commas; word-
boundary match).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fmt::join(range, delimiter) produces exactly MRtrix's delimiter-only output (no
brackets). Convert all ~40 MR::join call sites across cmd/, core/ and gui/:
- Where join() was a direct argument to an fmt-style sink (fmt::format, CONSOLE/
INFO/WARN/Exception, Option/Argument descriptions), inline it as fmt::join(...).
- Where a std::string result was required (keyval assignment, '+' concatenation,
stream insertion), wrap as fmt::format("{}", fmt::join(...)).
Remove the MR::join declarations/definitions (core/mrtrix.h, core/mrtrix.cpp),
including the unused null-terminated char*const* overload. core/enum.h's unrelated
detail::join is left intact. <fmt/ranges.h> (added in Task 6) supplies fmt::join.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Convert the ~30 C-style printf() call sites to fmt::format brace syntax,
translating the format specifiers: %04X->{:04X}, %8 PRIu64->{:8},
%3 PRI_SIZET %%->{:3}%, %.4g->{:.4g}, %5u->{:5}, %3.3f%%->{:3.3f}%,
%c%c->{}{}, %*.*d (width==precision==N)->{:0{}} (zero-pad to N digits).
- progressbar.cpp: ANSI WRAP_*/CLEAR_LINE_CODE literal macros concatenate into
the fmt format string unchanged (no braces inside them).
- exception.cpp: the per-colour format strings (held in a runtime map) become
brace templates rendered via fmt::format(fmt::runtime(...), ...).
- dicom/image.cpp: DEBUG(printf(...)) collapses to DEBUG("...{:04X}...", ...)
since DEBUG already takes an fmt format string.
Remove MR::printf and its -Wformat-security pragma block from core/mrtrix.h.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the column-vector auto-transpose in the fmtlib Eigen formatter
with an opt-in `T` spec character: `{:T}` transposes the value and appends
the `^T` suffix, while `{}` defers to Eigen's native ostream layout. The
thirteen call sites whose explicit `.transpose()` was stripped in commit
e177c00 (check_gradient.h, gradient_descent_bb.h, evaluate.h, the two
initialiser*.cpp files) now carry `:T` and so render identically to the
prior auto-transpose behaviour.
Rewrite `MR::to<T>()` on top of `std::from_chars` for arithmetic types,
retaining `std::istringstream` as a per-family fallback whenever the
standard library in use lacks the corresponding from_chars support.
CMake configure-time checks (`MRTRIX_HAVE_FROM_CHARS_INT`,
`MRTRIX_HAVE_FROM_CHARS_FP`) gate the dispatch. The all-input-consumed
invariant is preserved, and `errc::result_out_of_range` now surfaces as
a distinct overflow Exception rather than silently saturating. The bool
and complex specialisations are unchanged; the floating-point keyword
fallback (`nan`/`-nan`/`inf`/`-inf`) is shared between both backends.
Augment the unit tests with: an fmtlib<->MR::to<>() complex round-trip
driven directly off `fmt::format("{}", value)` output; rejection of
parenthesised inputs for real-valued conversions; and exhaustive
integer/float overflow detection. Two existing expectations
(`infinity`/`-infinity`) flip to accept, matching from_chars semantics
per C++17.
Session prompts:
1. > 1. In the custom Eigen fmtlib formatter, rather than auto-detecting
> that the input is a column vector and automatically printing its
> transpose and adding the "^T" suffix, instead implement a custom
> formatting specifier that nominates for the transpose of the input
> to be displayed. For all printing / macro calls involving Eigen
> classes that were refactored in a recent commit to remove the
> explicit call to .transpose(), insert use of the custom formatter.
> 2. In function MR::to<>(), change the underlying implementation from
> std::istringstream to C++17 std::from_chars(). Ensure that the
> check that all content of the input string was utilised in the
> conversion was used persists. Ensure that the unit tests for that
> function still pass. Ensure augmentation of these unit tests to
> prove that for specifically complex-valued outputs, the round
> brackets produced by fmtlib when writing complex numbers are
> handled appropriately, whereas for real-valued data the presence of
> such brackets is not permitted. Augment unit tests to ensure that
> datatype range overflow is suitably caught.
2. > In the transition to std::from_chars(), it will be necessary to add
> a test during configuration to make sure that the implementation of
> that function for the compiler in use supports all required data
> types; where it does not, the implementation must fall back via
> precompiler directives to the current implementation.
Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| private: | ||
| Image<complex_type> ℑ | ||
| Image<T> ℑ |
There was a problem hiding this comment.
warning: member 'image' of type 'Image &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
Image<T> ℑ
^|
|
||
| private: | ||
| Image<complex_type> ℑ | ||
| Image<T> ℑ |
There was a problem hiding this comment.
warning: member 'image' of type 'Image<complex> &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
Image<T> ℑ
^|
|
||
| private: | ||
| Image<complex_type> ℑ | ||
| Image<T> ℑ |
There was a problem hiding this comment.
warning: member 'image' of type 'Image &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
Image<T> ℑ
^| out << "Length,Sum_weights\n"; | ||
| for (size_t i = 0; i != histogram.size(); ++i) | ||
| out << str(i * step_size) << "," << str(histogram[i]) << "\n"; | ||
| out << fmt::format("{}", i * step_size) << "," << fmt::format("{}", histogram[i]) << "\n"; |
There was a problem hiding this comment.
warning: narrowing conversion from 'size_t' (aka 'unsigned long') to 'float' [bugprone-narrowing-conversions]
out << fmt::format("{}", i * step_size) << "," << fmt::format("{}", histogram[i]) << "\n";
^| out << "Length,Count\n"; | ||
| for (size_t i = 0; i != histogram.size(); ++i) | ||
| out << str(i * step_size) << "," << str<size_t>(histogram[i]) << "\n"; | ||
| out << fmt::format("{}", i * step_size) << "," << fmt::format("{}", static_cast<size_t>(histogram[i])) << "\n"; |
There was a problem hiding this comment.
warning: narrowing conversion from 'size_t' (aka 'unsigned long') to 'float' [bugprone-narrowing-conversions]
out << fmt::format("{}", i * step_size) << "," << fmt::format("{}", static_cast<size_t>(histogram[i])) << "\n";
^| root->appendChild(new TreeItem("VSync", format.swapInterval() ? "on" : "off", root)); | ||
| root->appendChild( | ||
| new TreeItem("Multisample anti-aliasing", format.samples() ? str(format.samples()).c_str() : "off", root)); | ||
| root->appendChild(new TreeItem( |
There was a problem hiding this comment.
warning: initializing non-owner argument of type 'TreeItem *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
root->appendChild(new TreeItem(
^| root->appendChild( | ||
| new TreeItem("Multisample anti-aliasing", format.samples() ? str(format.samples()).c_str() : "off", root)); | ||
| root->appendChild(new TreeItem( | ||
| "Multisample anti-aliasing", format.samples() ? fmt::format("{}", format.samples()).c_str() : "off", root)); |
There was a problem hiding this comment.
warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]
Suggested change
| "Multisample anti-aliasing", format.samples() ? fmt::format("{}", format.samples()).c_str() : "off", root)); | |
| "Multisample anti-aliasing", format.samples() != 0 ? fmt::format("{}", format.samples()).c_str() : "off", root)); |
| GLint max_2d_texture_size; | ||
| gl::GetIntegerv(gl::MAX_TEXTURE_SIZE, &max_2d_texture_size); | ||
| root->appendChild(new TreeItem("Maximum 2D texture size", str(max_2d_texture_size), root)); | ||
| root->appendChild(new TreeItem("Maximum 2D texture size", fmt::format("{}", max_2d_texture_size), root)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type 'TreeItem *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
root->appendChild(new TreeItem("Maximum 2D texture size", fmt::format("{}", max_2d_texture_size), root));
^| GLint max_3D_texture_size; | ||
| gl::GetIntegerv(gl::MAX_3D_TEXTURE_SIZE, &max_3D_texture_size); | ||
| root->appendChild(new TreeItem("Maximum 3D texture size", str(max_3D_texture_size), root)); | ||
| root->appendChild(new TreeItem("Maximum 3D texture size", fmt::format("{}", max_3D_texture_size), root)); |
There was a problem hiding this comment.
warning: initializing non-owner argument of type 'TreeItem *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
root->appendChild(new TreeItem("Maximum 3D texture size", fmt::format("{}", max_3D_texture_size), root));
^| } | ||
| if (index.column() == 0 && role == Qt::DisplayRole) | ||
| return qstr(str(index.row())); | ||
| return qstr(fmt::format("{}", index.row())); |
There was a problem hiding this comment.
warning: repeated branch in conditional chain [bugprone-branch-clone]
return qstr(fmt::format("{}", index.row()));
^Additional context
cpp/gui/mrview/tool/connectome/node_list.cpp:45: end of the original
return qstr(fmt::format("{}", index.row()));
^cpp/gui/mrview/tool/connectome/node_list.cpp:49: clone 1 starts here
return qstr(connectome.nodes[index.row()].get_name());
^
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
1 participant
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.
Potential extension of #3339.
Using that branch as the base here to evaluate changes given I've already gone to the effort of manually code-reviewing #3339 in its current form.
Draft for now; deferring to a better time for me to review manually.
Had a go at further reducing redundancies between legacy MRtrix string-handling functions and
fmtlibcapabilities.MR::str()function, but it just wrapsfmt::format("{}", item).<<operators are involved, collapse the whole operation into a singlefmt::format()call.static_cast<>calls onMR::Helper::IndexandMR::Helper::Value.MR::printf()usages, confirm column vector transposition display)