Skip to content

Conversation

@daljit46
Copy link
Member

@daljit46 daljit46 commented May 23, 2023

This concatenates nested namespaces which is a features that has been introduced in C++17. It partially addresses #2585.
The changes were made with the help of Bear and clang-tidy. I haven't bothered with formatting yet, but we may want to automate this too using something like clang-format.

@jdtournier
Copy link
Member

Thanks @daljit46! I have a feeling this may best be done under the authorship of @MRtrixBot though... What do you reckon, @Lestropie?

@Lestropie
Copy link
Member

  1. I think it would be better to have a cpp17 branch based off of dev. That can have both the changes to scripts / documentation to make C++17 the default, and serve as the target branch for PRs involving the adoption of individual C++17 features. That would keep distinct C++17 features separate from one another, and would eventually result in one separate PR merging all C++17-relevant content into dev.

  2. Anything that involves automated mass generation of line changes should ideally be authored by @MRtrixBot, in order to prevent distortion of contribution statistics. There are some historical commits that predate this idea, but we try to stick with this approach now. It's not a stringent rule, eg. auto-recompiling documentation to reflect one's code changes is fine to include under one's own account, but when there's a gross discrepancy between line change count and effort invested to generate those lines it should ideally go under @MRtrixBot.

  3. What I will typically try to do myself here is to contribute any corresponding code / CI tests under my own account, and then attribute just the bulk resulting change to @MRtrixBot. So for instance here, rather than just updating the current code content, the script responsible for making the change could be executed as CI to ensure that nested namespace use does not reappear in the future. And addition of the script / CI configuration to do so would be under default authorship.

  4. The above also in my experience provides an appropriate bifurcation with respect to what should vs. should not go into .git-blame-ignore-revs (Add file ".git-blame-ignore-revs" #2592). So eg. the changes to the namespace definitions themselves should appear in blame, but the corresponding indentation changes should not.

  5. The primary motivation for using nested namespaces is the reduction of global indentation; so there wouldn't be much benefit in 73757de until we also deal with the formatting question.

  6. We've previously tinkered with the prospect of more general automated code formatting and enforcement through CI. Given the need to modify indentation across the entire code base, it's a pertinent discussion to re-raise. At the time we had concluded that contributors should be free to conform to their own formatting preferences. Whether that decision remains post-production-release and with the evolution of formatting tools within them might be up to debate. Personally I'm content to retain that philosophy. Just don't know if it will be possible to automatically modify only the bulk indentation and nothing else, or whether it'll need to be a shift-select shift-tab individually for each source file.

@daljit46
Copy link
Member Author

daljit46 commented May 24, 2023

For the formatting, I think it might be a good idea to not format the code when merging this change. Formatting will lead to merge conflicts (since we are planning to merge other branches like #2620). Instead, we could merge a branch with changes to namespaces only and only subsequently merge one that contains the necessary formatting changes.

More generally, I would be in favour of a standard formatting convention that uses clang-format. It would provide consistency and a clear constraint on how developers are ought to format their code (with no room for arguments on style). I see very little benefit in giving the developers the possibility to introduce all kind of different formatting conventions and it's not pretty to see commits changing the formatting of unrelated code (especially when a pull request is the work of multiple contributors).

@daljit46 daljit46 force-pushed the nested_namespaces branch from 4a1c5ce to 64e906b Compare May 26, 2023 14:00
@daljit46 daljit46 force-pushed the nested_namespaces branch 2 times, most recently from 75ba90e to 94cefcf Compare February 26, 2024 17:34
@daljit46
Copy link
Member Author

This seems ready to be merged now that we have implemented automatic formatting rules via #2652.

@daljit46 daljit46 self-assigned this Feb 26, 2024
@daljit46 daljit46 requested a review from a team February 26, 2024 17:40
@daljit46 daljit46 marked this pull request as ready for review February 26, 2024 17:41
@daljit46
Copy link
Member Author

@Lestropie actually it probably make sense to wait for #2818 to be merged before.

@daljit46 daljit46 merged commit 0d55bb5 into dev Feb 27, 2024
@daljit46 daljit46 deleted the nested_namespaces branch February 27, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants