Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply formatting for C++ using clang-format #2652

Merged
merged 11 commits into from
Aug 24, 2023
Merged

Apply formatting for C++ using clang-format #2652

merged 11 commits into from
Aug 24, 2023

Conversation

daljit46
Copy link
Member

@daljit46 daljit46 commented Jun 9, 2023

This pull request will introduce clang-format as the formatting tool for our C++ code. As discussed in #2643, this will provide consistency and a uniform formatting way for all developers.
It contains a new CI workflow that checks whether new code is formatted correctly. It also contains a pre-commit configuration file to install a local git hook, which developers can use to ensure that their code is properly formatted before pushing.
Formatting has already been applied to all C++ files using the LLVM style with some custom settings (still needs tweaking).

Any suggestions or feedback is very welcome!

@daljit46 daljit46 force-pushed the clangformat branch 9 times, most recently from 2ba33a0 to a15b144 Compare June 21, 2023 13:46
@daljit46
Copy link
Member Author

daljit46 commented Jun 21, 2023

OK, I think this looks quite good now, and I think it's not a bad idea to merge this directly into dev. There are few things that need to be considered. Since we are enforcing the 100 words character limit, the scripts for generating the documentation and updating the copyright notice were broken. For the former, I directly updated the related rst files and adjusted the regular expressions needed to parse the configuration options and the environment variables in the code. For the latter, I adjusted the update_copyright script to update the copyright notices correctly. It'd be good to review these changes (last three commits in the pull request).

Other than that, everything looks good.

@daljit46 daljit46 marked this pull request as ready for review June 21, 2023 13:59
@Lestropie
Copy link
Member

🤦‍♂️ I'd written a bunch of stuff on this PR at some point, but clearly didn't hit the send button, and now can't find it unsubmitted on either system... I'll have another go, I can probably remember some of the code snippets I didn't like.

Regarding destination branch / merge timing: I'd commented on Teams and in #2655 that there's a bunch of proposed modifications that aren't C++17 and therefore aren't #2585 / cpp17 branch, but nevertheless fall into some kind of category of "back-end cleanup"; eg. #2111, arguably #2579 due to its interaction with this PR. An alternative proposal would be that just like a central cpp17 branch was created to act as the target for all C++17-related PRs to eventually all be merged to dev in one go, there could be considered to be another set of non-C++17 software eng modifications with its own target branch. Not necessarily pushing for it as it's a fairly loose grouping, just throwing it out there.

@daljit46
Copy link
Member Author

daljit46 commented Jun 22, 2023

I think out of the c++17 changes, the only one that makes sense to deliver together with this PR is #2641. The way I see it is that this is (mostly) a formatting change, thus it shouldn't modify the behaviour of the code, while for example #2111 is a change that will affect the generated code (and most likely cannot really be automated because each macro usage will have to be evaluated before switching to constexpr).
#2579 could also be a good candidate if we decide to create a relevant super PR that includes this one, although I think we need to test that change (in the past, I've had quite a few issues with include-what-you-use when dealing with system headers), so I'll have a go at that.

@Lestropie
Copy link
Member

  1. Commit changing formatting needs to go into .git-blame-ignore-revs.

  2. Still debate to be had about the character limit. Personally I'd rather have code that is a little wide, maybe doesn't display quite right on some people's systems in some contexts, and maybe draws an explicit request to split, rather than a hard limit less than a quarter the width of my screen that I have to sacrifice readability of code logic or variable names to conform to.
    (I'd joked to @daljit46 that maybe we should use a limit of 132 characters.... let's see if @jdtournier gets the reference)

  3. Many text strings are poorly split when done automatically, and we may want to go through and manually clean a few up.

  4. In usage() of cmd/*.cpp, the positioning of operators & spaces around them is really not great. This might be due to being an unconventional use of classes or something. But I'd like to try to modify that without breaking formatting elsewhere.

  5. Ternary operators seem to appear at line start rather than line end, which is alien to me; but I'd get used to it if need be.

  6. In core/adapter/extract.h I see:

    Extract(const ImageType &original, const vector<vector<uint32_t>> &indices)
          : base_type(original), current_pos(ndim()), indices(indices), trans(original.transform()) {
    

    Here I think I'd rather PackConstructorInitializers: Never

    See also:

    Gaussian1D(const ImageType &parent,
                 default_type stdev_in = 1.0,
                 size_t axis_in = 0,
                 size_t extent = 0,
                 bool zero_boundary = false)
          : base_type(parent), stdev(stdev_in), axis(axis_in), zero_boundary(zero_boundary) {
    

    Initializers often shadow constructor parameters, so having the latter split but the former not just looks weird.

  7. In function paragraph() defined in core/app.*, it splits the return type onto a newline rather than splitting the function parameters. Does it treat naked functions different to class member functions?

(as far as I'm getting this evening; I might continue looking through more exemplar source files at a later date)

@Lestropie
Copy link
Member

Some examples of where stringently enforced line width causes ugliness:

Old:

template <typename X>
    struct max_digits<X, typename std::enable_if<std::is_fundamental<typename X::Scalar>::value, int>::type> { 
      static constexpr int value () { return std::numeric_limits<typename X::Scalar>::max_digits10; }
    };

New:

template <typename X>
struct max_digits<
    X,
    typename std::enable_if<std::is_fundamental<typename X::Scalar>::value, int>::type> {
  static constexpr int value() { return std::numeric_limits<typename X::Scalar>::max_digits10; }
};

Old:

  //! check whether type is compatible with MRtrix3's file IO backend:
  template <class ValueType>
    struct is_data_type :
      std::integral_constant<bool, std::is_arithmetic<ValueType>::value || is_complex<ValueType>::value> {  };

New:

//! check whether type is compatible with MRtrix3's file IO backend:
template <class ValueType>
struct is_data_type
    : std::integral_constant<bool,
                             std::is_arithmetic<ValueType>::value || is_complex<ValueType>::value> {
};

@daljit46 daljit46 force-pushed the clangformat branch 2 times, most recently from 1d2ab75 to 5c24401 Compare August 17, 2023 12:41
@daljit46 daljit46 force-pushed the clangformat branch 2 times, most recently from c3911ee to 9a3357b Compare August 17, 2023 14:50
@daljit46 daljit46 force-pushed the clangformat branch 5 times, most recently from c10c460 to 6ebc0dc Compare August 20, 2023 10:07
@daljit46 daljit46 force-pushed the clangformat branch 3 times, most recently from dc211e4 to f109307 Compare August 21, 2023 11:50
@daljit46
Copy link
Member Author

daljit46 commented Aug 21, 2023

I've added a new script called clang-format-all in the top level directory, which formats all C++ code in the source directory (excluding 3rdparty headers). The script also allows you to specify a custom clang-format binary (e.g. ./clang-format-all -e custom_clang_format) so you can always a specific version of clang-format. This should ease the process of migrating existing PRs to our new formatting rules.

I think this can now be merged in. This will require all existing and future PRs to format their code with clang-format in order to pass our CI checks.

@daljit46
Copy link
Member Author

@MRtrix3/mrtrix3-devs unless anyone have any reservations, I will merge this into dev soon.

@daljit46 daljit46 merged commit 5b0ec65 into dev Aug 24, 2023
6 checks passed
@daljit46 daljit46 deleted the clangformat branch August 24, 2023 07:37
@Lestropie Lestropie mentioned this pull request Feb 20, 2024
Lestropie added a commit that referenced this pull request Feb 21, 2024
Resolves changes proposed in e073be5 (#2700) against introduction of clang-format (#2652).
Lestropie added a commit that referenced this pull request Feb 21, 2024
Resolves changes proposed in e073be5 (#2700) against introduction of clang-format (#2652).
Lestropie added a commit that referenced this pull request Feb 21, 2024
When clang-format, introduced in #2652, applied its changes to MRtrix3 code relating to the command-line interface (particularly the way that options and their arguments are concatenated through the addition operator), the resulting code was often quite ugly. This commit replaces code relating to CLI configuration (both the usage() function in cmd/*.cpp, and in definitions of options and option groups utilised by multiple commands) with manually formatted code, and includes comment fields to disable further manipulation of that code by clang-format.
Lestropie pushed a commit that referenced this pull request Feb 26, 2024
When clang-format, introduced in #2652, applied its changes to MRtrix3 code relating to the command-line interface (particularly the way that options and their arguments are concatenated through the addition operator), the resulting code was often quite ugly. This commit replaces code relating to CLI configuration (both the usage() function in cmd/*.cpp, and in definitions of options and option groups utilised by multiple commands) with manually formatted code, and includes comment fields to disable further manipulation of that code by clang-format.
This commit is a refactor of 0cec87a generated as part of #2815, with the changes being to omit other modifications to the command documentation that are not directly related to clang-format (those changes are in the parent commit to this commit: 15e253c), and to change authorship of the commit.
Lestropie added a commit that referenced this pull request Sep 17, 2024
- Resolves addition of peaksconvert command in #2918 with API changes in #2911.
- Resolves with clang-format added in #2652.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants