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

Prep-work for clang-format. #3078

Closed
wants to merge 4 commits into from
Closed

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 7, 2021

Prep-work for global clang-format (see experimental/demo PRs #3074, #3082).

NO bulk changes in this PR, just minimal changes such that a global clang-format is stable after a single pass.

This was several hours of tedious work running clang-format & clang-tidy until both are stable. Breaking out as a separate PR so that these manual changes do not have to be maintained separately while we decide how to integrate clang-format or clang-format-diff into the pre-commit hooks.

Notable steps:

  • Systematically adding // clang-format off, on around all \rst \endrst blocks. — Possibly some could be omitted, but for blocks that are deliberately formatted as \rst it is safer to systematically exclude them from clang-format. Note that the sphinx error messages are often completely useless if something goes wrong, therefore letting clang-format loose on \rst blocks is likely to turn into time-consuming surprises.

  • Removing stray semicolons (discovered by running clang-format v12 followed by tools/check-style.sh).

  • Manually moving clang-tidy // NOLINT and // NOLINTNEXTLINE comments so that clang-format does not move them to the wrong places. (This was the most tedious part.)

  • Manually reformatting comments related to static_asserts so that clang-format does not need two passes.

  • git diff -U0 --no-color HEAD^ | python3 $HOME/clone/llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -style=file -i

Suggested changelog entry:

Prep-work for adding clang-format to the pre-commit hooks.

@rwgk rwgk marked this pull request as draft July 7, 2021 00:50
Ralf W. Grosse-Kunstleve added 4 commits July 9, 2021 07:15
* Inserting clang-format using the same docker container as used for clang-tidy (silkeh/clang:12).

* Removing stray semicolons (discovered by running clang-format v12 followed by tools/check-style.sh).

* Manually moving `// NOLINT` and `// NOLINTNEXTLINE` comments so that clang-format does not move them to the wrong places.

* Manually reformatting comments related to `static_assert`s so that clang-format does not need two passes.

* git diff -U0 --no-color HEAD^ | python3 $HOME/clone/llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -style=file -i
… the order and thereby confusing MSVC 2015.
…r the just-merged PR pybind#3080. Also rerunning clang-format-diff.py to pick up a few fixes missed before.
@rwgk rwgk force-pushed the prework_for_clang_format branch from 98e451b to eb0a55b Compare July 9, 2021 14:23
@rwgk rwgk marked this pull request as ready for review July 9, 2021 18:20
@@ -105,6 +116,7 @@ class object_api : public pyobject_tag {
function will throw a `cast_error` exception. When the Python function
call fails, a `error_already_set` exception is thrown.
\endrst */
// clang-format on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't disabling reflow_comments be better? Or there may be some of there might be some way to get it to recognize \rst \endrst as off and on clang-format comments.

tests/test_builtin_casters.cpp Show resolved Hide resolved
tests/test_kwargs_and_defaults.cpp Show resolved Hide resolved
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 12, 2021

This one I will basically have to do from scratch after #3087 is done. At that point the only remaining clang-format related question should be how to deal with the \rst blocks.

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 15, 2021

This PR is obsolete. The only step left to do on top of the merged #3087 is somehow dealing with the \rst \endrst comment blocks. I have a simple script that inserts // clang-format off, on automatically, used for the experimental/demo draft PR #3074. There is no need for a manual step as far as I can predict, therefore this PR is obsolete.

@rwgk rwgk closed this Jul 15, 2021
@rwgk rwgk deleted the prework_for_clang_format branch July 15, 2021 19:02
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.

2 participants