Conversation
Note that operator+(StringType, StringType) has been removed;
fmt::format() needs to be adopted wherever string concatenation is required.
Prompts:
Start by porting all Exception messages and debug / terminal macro usages to use fmt::format().
Detect all circumstances where the fmtlib::fmt "{}" syntax is used within the format specification of a fmt::format() invocation to insert a constexpr string. Move the constexpr string content into the format specification.
Audit local unstaged changes for string constructions where the printing of a variable's content has been removed, substituted with reduced information, or substituted with an explicit cast to string before passing to fmt::format(), due to the absence of a custom formatter. Tabulate the set of relevant classes and the number of such modifications for each.
Remove all redundant typecasts within fmt::format() calls.
Find all attempts to pass an MR::Exception as an argument to fmt::format(). Instead of attempting to insert a string formatting of the existing MR::Exception into the message of a new MR::Exception, instead use the constructor of MR::Exception where the first input argument is an existing Exception and the second argument is a message string.
Remove explicit casts of MR::ParsedArgument to std::string for compatibility with fmt::format by making use of the proposed custom formatter in cpp/core/fmt.h.
In uses of fmt::format(), replace double-backslahes before double-quotes with appropriate single-backslash escape character.
In all MR::Exception constructor invocations where the input message argument is not a constexpr string but includes the presentation of runtime data, use fmt::format() to construct the string rather than operator+() on std::strings. Where an MR::Exception takes as the first input to its constructor another MR::Exception, leave this unaltered.
Look at all instance of "throw Exception". If the input message is formed using the addition operator, instead create it using the fmt::format() funciton.
Iteratively attempt to compile the software. Any error resulting from an attempt to invoke an addition operator for string types (std::string, std::string_view, char[], char*) should be replaced with an invocation of fmt::format().
Iteratively fix existing utilisation of fmt::format() until compliation succeeds.
Try to use the custom formatters in cpp/core/fmt.h to enable passing Eigen data through the fmt::format() function, modifying as needed.
Propose one or more custom formatters for Eigen::Dense classes in cpp/cmd/fmt.h (justifying why different formatters should be used for different cases), and attempt to recompile while making use of them.
Perform an audit of the current state of refactoring of the C++ repository to make use of the fmtlib::fmt library. Attempt to restore compilation. Attempt to make use of a custom formatter for fmtlib compatibility per each non-trivial-data-type class, creating new formatters where required. Remove redundant invocations of MR::str() and explicit casts to std::string within fmt::format() calls. Quantify remaining prevalence of use of operator+() to concatenate strings.
Propose a modification to the custom fmtlib parser in cpp/cmd/fmt.h for Eigen matrices that would make it also with for Eigen::Block.
Custom fmtlib formatter for Eigen dense expressions at cpp/cmd/fmt.h:49 still does not work for Eigen::Block despite use of std::remove_cv_t<> to deal with differences in constness. Propose another solution to make this template function applicable to Eigen::Block.
Propose alterations to the custom fmtlib formatter in cpp/cmd/fmt.h so that the current function printing the contents of 1D vectors and 2D matrices works only for dense matrix types and Eigen::Block, but not for Eigen::Transform.
Confirm Eigen::Block derives from Eigen::DenseBase.
I changed the existing fustom formatter to use std::is_base_of_v<Eigen::DenseBase<>> rather than std::enable_if_t<MR::is_eigen_type<>>, and it does not compile for Eigen::Block, conflicting with your response.
Clarification: the code not compiling was already based on std::is_base_of_v<Eigen::DenseBase<std::remove_cv_t<Derived>>, std::remove_cv_t<Derived>>>; I misleadingly omitted for brevity. The first compiler error is: /home/unimelb.edu.au/robertes/src/worktrees/fmt/build_debug/_deps/fmt-src/include/fmt/base.h:1625:63: error: implicit instantiation of undefined template 'fmt::detail::type_is_unformattable_for<Eigen::Block<Eigen::Matrix<double, 3, 4, 0, 3, 4>, 3, 1, true>, char>'.
Delete function operator+() at cpp/cmd/mrtrix.h:33. Fix resulting compilation failures by replacing use of operator+() for string concatenation with use of fmt::format. One line of code / one line of new-line-delimited text should ideally contain only one invocation of fmt::format(), with the format specifier containing all string substitutions for that line. Remove redundant invocations of str() when passing variables to this function. Remove invocations of .transpose() for Eigen column vectors. List any types for which passing directly to fmt::format() does not compile due to absence of a custom formatter.
Co-authored-by: Claude <noreply@anthropic.com>
Prompt: 1. If a constexpr string and a result of fmt::format() are concatenated via operator+(), move the entire constexpr string content into the format specifier as the first input argument to fmt::format(). 2. If an output from function MR::str() is passed as an input argument to fmt::format(), remove the str() call and pass the argument directly. If this results in compilation failure, report the type of the relevant variable as a candidate for creation of a custom formatter. 3. If any input argument to fmt::format() is a constexpr string, move that content into the format specification (being the first input argument to that function). 4. if fmt::format() is used to produce one of the input arguments to fmt::format(), refactor unless there is a strong justification to preserve the current code structure (eg. ternary operators involved). 5. Investigate why there are still string concatenation operations occurring using operator+(). If this is making use of an MRtrix3 function, present that function as a candidate for deletion. If this is a language feature, find and replace all instances of use of that operator with a fmt::format() composition. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Across 21 files, apply five categories of clean-up to the ongoing fmtlib
migration. String-literal operands to operator+ are folded into the
adjacent fmt::format() call. MR::printf() calls whose specifiers all have
fmtlib equivalents are replaced with fmt::format(), including DICOM display
routines and dirmerge's output loop. Erroneous double-backslash escape
sequences (\\n, \\t) inside format strings are corrected to their
single-escape forms. String-literal arguments passed to fmt::format() are
moved directly into the format string, reducing argument count and improving
readability.
Session prompts:
1. > 1. In all instances where operator+() is used to concatenate a string
> literal with the output of fmt::format(), envelop the preceding
> string literal into the fmt::format() format stirng.
> 2. If fmt::format() is called with a format string of "{}", and the
> input argument is itself the output of a call to fmt::format(),
> collapse the redundant call.
> 3. For all invocations of MR::printf() or std::printf(), if all
> conversion specifications have an equivalent in fmtlib, replace
> the printf() call with a call to fmt::format().
> 4. Find and fix instances of erroneous double-backslashes that are
> supposed to be character escapes for quite symbols or newlines as
> part of string literals.
> 5. If an input argument to fmt::format() is a string literal, move
> that literal into the format string.
Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
One nice side-effect I've noticed with use of this library is that elongated strings due to floating-point imprecision (eg. user specifies "0.1", metadata encodes "0.0999996") seem to be mitigated. |
Conflicts: cmake/Dependencies.cmake cpp/cmd/5tt2vis.cpp cpp/cmd/5ttedit.cpp cpp/cmd/5ttvalidate.cpp cpp/cmd/amp2response.cpp cpp/cmd/amp2sh.cpp cpp/cmd/connectome2tck.cpp cpp/cmd/connectomeedit.cpp cpp/cmd/connectomestats.cpp cpp/cmd/dcminfo.cpp cpp/cmd/dirflip.cpp cpp/cmd/dirgen.cpp cpp/cmd/dirmerge.cpp cpp/cmd/dirrotate.cpp cpp/cmd/dirsplit.cpp cpp/cmd/dirstat.cpp cpp/cmd/dirvalidate.cpp cpp/cmd/dwi2adc.cpp cpp/cmd/dwi2fod.cpp cpp/cmd/dwidenoise.cpp cpp/cmd/dwirecon.cpp cpp/cmd/fixel2peaks.cpp cpp/cmd/fixel2tsf.cpp cpp/cmd/fixel2voxel.cpp cpp/cmd/fixelcfestats.cpp cpp/cmd/fixelconnectivity.cpp cpp/cmd/fixelcorrespondence.cpp cpp/cmd/fixelfilter.cpp cpp/cmd/fixeltransform.cpp cpp/cmd/fixelvalidate.cpp cpp/cmd/fod2dec.cpp cpp/cmd/fod2fixel.cpp cpp/cmd/label2mesh.cpp cpp/cmd/labelconvert.cpp cpp/cmd/labelstats.cpp cpp/cmd/maskdump.cpp cpp/cmd/maskfilter.cpp cpp/cmd/meshconvert.cpp cpp/cmd/meshfilter.cpp cpp/cmd/meshvalidate.cpp cpp/cmd/mraverageheader.cpp cpp/cmd/mrcalc.cpp cpp/cmd/mrcat.cpp cpp/cmd/mrcheckerboardmask.cpp cpp/cmd/mrclusterstats.cpp cpp/cmd/mrconvert.cpp cpp/cmd/mredit.cpp cpp/cmd/mrfilter.cpp cpp/cmd/mrgrid.cpp cpp/cmd/mrhistmatch.cpp cpp/cmd/mrhistogram.cpp cpp/cmd/mrinfo.cpp cpp/cmd/mrmath.cpp cpp/cmd/mrmetric.cpp cpp/cmd/mrregister.cpp cpp/cmd/mrthreshold.cpp cpp/cmd/mrtransform.cpp cpp/cmd/mtnormalise.cpp cpp/cmd/peaks2fixel.cpp cpp/cmd/peaksconvert.cpp cpp/cmd/sh2peaks.cpp cpp/cmd/sh2response.cpp cpp/cmd/shconv.cpp cpp/cmd/tck2fixel.cpp cpp/cmd/tckdfc.cpp cpp/cmd/tckedit.cpp cpp/cmd/tckglobal.cpp cpp/cmd/tckmap.cpp cpp/cmd/tckresample.cpp cpp/cmd/tcksample.cpp cpp/cmd/tcksift2.cpp cpp/cmd/tckstats.cpp cpp/cmd/tckvalidate.cpp cpp/cmd/tensor2metric.cpp cpp/cmd/transformcalc.cpp cpp/cmd/transformcompose.cpp cpp/cmd/transformconvert.cpp cpp/cmd/tsfdivide.cpp cpp/cmd/tsfmult.cpp cpp/cmd/tsfvalidate.cpp cpp/cmd/vectorstats.cpp cpp/cmd/warpconvert.cpp cpp/cmd/warpcorrect.cpp cpp/core/CMakeLists.txt cpp/core/algo/histogram.cpp cpp/core/app.cpp cpp/core/app.h cpp/core/connectome/lut.cpp cpp/core/connectome/validate.cpp cpp/core/dwi/directions/file.cpp cpp/core/dwi/directions/set.h cpp/core/dwi/directions/validate.cpp cpp/core/dwi/gradient.cpp cpp/core/dwi/gradient.h cpp/core/dwi/sdeconv/csd.h cpp/core/dwi/sdeconv/msmt_csd.h cpp/core/dwi/tractography/ACT/act.cpp cpp/core/dwi/tractography/ACT/validate.cpp cpp/core/dwi/tractography/SIFT/model.h cpp/core/dwi/tractography/SIFT/model_base.h cpp/core/dwi/tractography/SIFT/sifter.cpp cpp/core/dwi/tractography/SIFT2/tckfactor.cpp cpp/core/dwi/tractography/algorithms/iFOD1.h cpp/core/dwi/tractography/algorithms/iFOD2.h cpp/core/dwi/tractography/connectome/connectome.cpp cpp/core/dwi/tractography/connectome/metric.h cpp/core/dwi/tractography/editing/receiver.h cpp/core/dwi/tractography/file.h cpp/core/dwi/tractography/file_base.cpp cpp/core/dwi/tractography/file_base.h cpp/core/dwi/tractography/mapping/writer.h cpp/core/dwi/tractography/roi.cpp cpp/core/dwi/tractography/roi.h cpp/core/dwi/tractography/scalar_file.h cpp/core/dwi/tractography/seeding/basic.cpp cpp/core/dwi/tractography/seeding/dynamic.cpp cpp/core/dwi/tractography/tracking/exec.h cpp/core/dwi/tractography/tracking/shared.cpp cpp/core/dwi/tractography/validate.cpp cpp/core/enum.h cpp/core/file/config.cpp cpp/core/file/copy.h cpp/core/file/dicom/csa_entry.h cpp/core/file/dicom/element.cpp cpp/core/file/dicom/element.h cpp/core/file/dicom/image.cpp cpp/core/file/dicom/mapper.cpp cpp/core/file/dicom/quick_scan.cpp cpp/core/file/dicom/tree.cpp cpp/core/file/gz.h cpp/core/file/json_utils.cpp cpp/core/file/key_value.cpp cpp/core/file/matrix.h cpp/core/file/mgh.h cpp/core/file/mmap.cpp cpp/core/file/name_parser.cpp cpp/core/file/nifti_utils.cpp cpp/core/file/npy.cpp cpp/core/file/ofstream.cpp cpp/core/file/path.cpp cpp/core/file/png.cpp cpp/core/file/utils.cpp cpp/core/filter/connected_components.cpp cpp/core/filter/connected_components.h cpp/core/fixel/filter/connect.cpp cpp/core/fixel/filter/smooth.cpp cpp/core/fixel/helpers.h cpp/core/fixel/matrix.cpp cpp/core/fixel/validate.cpp cpp/core/formats/dicom.cpp cpp/core/formats/mri.cpp cpp/core/formats/mrtrix.cpp cpp/core/formats/mrtrix_gz.cpp cpp/core/formats/mrtrix_utils.cpp cpp/core/formats/par.cpp cpp/core/formats/png.cpp cpp/core/formats/xds.cpp cpp/core/gpu/gpu.cpp cpp/core/header.cpp cpp/core/header.h cpp/core/image.h cpp/core/image_io/base.cpp cpp/core/image_io/default.cpp cpp/core/image_io/gz.cpp cpp/core/image_io/mosaic.cpp cpp/core/image_io/pipe.cpp cpp/core/image_io/png.cpp cpp/core/image_io/ram.cpp cpp/core/image_io/scratch.cpp cpp/core/image_io/variable_scaling.cpp cpp/core/math/stats/glm.cpp cpp/core/math/stats/import.h cpp/core/math/stats/shuffle.cpp cpp/core/metadata/phase_encoding.cpp cpp/core/metadata/phase_encoding.h cpp/core/metadata/slice_encoding.cpp cpp/core/misc/voxel2vector.h cpp/core/mrtrix.cpp cpp/core/mrtrix.h cpp/core/registration/linear.h cpp/core/registration/metric/params.h cpp/core/registration/multi_contrast.cpp cpp/core/registration/multi_contrast.h cpp/core/registration/nonlinear.h cpp/core/surface/freesurfer.cpp cpp/core/surface/mesh.cpp cpp/core/surface/mesh_multi.cpp cpp/core/surface/scalar.cpp cpp/gui/dialog/file.cpp cpp/gui/dialog/image_properties.cpp cpp/gui/mrview/displayable.h cpp/gui/mrview/tool/base.h cpp/gui/mrview/tool/connectome/connectome.cpp cpp/gui/mrview/tool/fixel/base_fixel.cpp cpp/gui/mrview/tool/fixel/fixel.cpp cpp/gui/mrview/tool/fixel/image4D.cpp cpp/gui/mrview/tool/odf/item.cpp cpp/gui/mrview/tool/odf/model.cpp cpp/gui/mrview/tool/overlay.cpp cpp/gui/mrview/tool/roi_editor/roi.cpp cpp/gui/mrview/tool/tractography/tractogram.cpp cpp/gui/mrview/tool/tractography/tractography.cpp cpp/gui/mrview/window.cpp cpp/gui/shview/render_window.cpp docs/reference/commands/5ttvalidate.rst testing/tools/testing_cpp_cli.cpp testing/tools/testing_diff_dir.cpp testing/tools/testing_diff_fixel.cpp testing/tools/testing_diff_matrix.cpp testing/tools/testing_diff_mesh.cpp testing/tools/testing_npyread.cpp Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The session introduced a parameterised GTest suite for MR::shorten(const std::filesystem::path &) covering three behavioural groups: passthrough (path fits within the limit), filename-only return (filename barely fits with no room for directory context), and middle-directory elision across both absolute and relative paths. Session prompts: 1. > Write a unit test file for new function > MR::shorten(const std::filesystem::path &). Ensure that > test paths cover a spectrum of total lengths, number of > directories, brevity of filename vs. directory names, > presence vs. absence of root name. Generated-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Following adoption of fmtlib for string formatting, most invocations of macros CONSOLE() / INFO() / DEBUG() / WARN() / FAIL() and construction of MR::Exception instances would involve invocation of fmt::format(), which made code unnecessarily verbose. This change embeds the fmt::format() call within those functions, so that invoking code can just provide the formatting string followed by any input arguments to the formatter. Session prompts: 1. > Find all locations in the repository where one of the macros > FAIL(), WARN(), INFO(), or CONSOLE() is used, or an MR::Exception > is constructed. Instead of function fmt::format() being used at > each instance to construct a message string, that function call > will instead be embedded within the terminal macro / the > MR::Exception constructor, using variadic macros / variadic > template constructor. Ensure that all such functionalities still > work even if no arguments are provided to the macro / > constructor, ie. the first input argument being a string does not > contain any substitutions. Ensure that fmt::format() is never > unnecessarily invoked explicitly in forming the inputs to any of > these macros / constructor. Ensure that MR::str() is never > invoked explicitly in forming the inputs to any of these macros > / constructor. Generated-by- Claude Opus 4.7 <noreply@anthropic.com>
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.
Supersedes #3125.
Cluses #2951.
Closes #3319.
Posting as draft PR to facilitate side-by-side review via GUI.
List below not necessarily exhaustive.
operator+()concatenation between a string constant andfmt::format(); would prefer that all text go into a singlefmt::format()call.fmt::format()as arguments rather than as part of the format specification.fmt::format()nesting.MR::str()as input arguments tofmt::format().fmt::format()call into terminal macros andMR::Exceptionconstructor.