Global clang-format #3074
Global clang-format #3074
Conversation
|
@rwgk Could you hold off on this for a sec, there are some readability clang-tidy checks that will also enforce LLVM style more strictly and it would be better to be one massive formatting diff than two. |
Absolutely: see the updated PR description. |
|
Note to self: reverting the clang-format changes in these files did NOT resolve the MSVC 2015 failures: |
| } | ||
|
|
||
| int closeFile = 1; | ||
| std::string fname_str = (std::string) fname; | ||
| #if PY_VERSION_HEX >= 0x03040000 | ||
| # if PY_VERSION_HEX >= 0x03040000 |
There was a problem hiding this comment.
Grr, is this the style we really want here?
There was a problem hiding this comment.
Good question.
I was thinking of formatting globally in different ways (multiple PRs), then we can look at the results holistically.
BTW: I'm still hung up on one last (I hope) thing, MSVC 2015 failures after clang-format.
There was a problem hiding this comment.
BTW: I fixed the MSVC 2015 issue.
To your style question, there are only 3 choices: https://clang.llvm.org/docs/ClangFormatStyleOptions.html
PPDIS_None, PPDIS_AfterHash, PPDIS_BeforeHash
What we have (AfterHash) seems like the most OK-ish option to me.
But really, any of these is better than having to mess around with the whitespaces manually.
| item_accessor operator[](handle key) const; | ||
| /// See above (the only difference is that they key is provided as a string literal) | ||
| item_accessor operator[](const char *key) const; | ||
|
|
||
| // clang-format off |
There was a problem hiding this comment.
To protect the \rst to \endrst block enclosed by the // clang-format off, on.
I added the clang-format meta-comments systematically rather than trying to figure out case by case if they are needed or not. Possibly some could be omitted, but I believe for blocks that are deliberately formatted as rst it's better to systematically exclude them from clang-format.
Note that the sphinx error messages are completely useless if something goes wrong. I spent quite a bit of time figuring out why the Documentation GitHub action was failing. Now I know, but letting clang-format loose on rst is likely to turn into time-consuming surprises for others.
rwgk
force-pushed
the
global_clang_format
branch
2 times, most recently
from
fc6ebd0
to
7585337
Compare
rwgk
force-pushed
the
global_clang_format
branch
2 times, most recently
from
d793a1f
to
c9610a9
Compare
…ollowed by tools/check-style.sh). * Manually moving `// NOLINT` 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. * Empty lines between #includes, to prevent clang-format from shuffling the order and thereby confusing MSVC 2015. * git diff -U0 --no-color HEAD^ | python3 $HOME/clone/llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -style=file -i
rwgk
force-pushed
the
global_clang_format
branch
from
fb55ca9
to
e2089eb
Compare
|
This PR is completely stale, even as a demo. The only slightly valuable commit is in the diff below. Closing & deleting branch. diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 133eb6df4e..6af90465ae 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -12,6 +12,10 @@
#
# See https://github.com/pre-commit/pre-commit
+ci:
+ skip:
+ - docker-clang-format
+
repos:
# Standard hooks
- repo: https://github.com/pre-commit/pre-commit-hooks
@@ -108,6 +112,19 @@ repos:
entry: PyBind|Numpy|Cmake|CCache
exclude: .pre-commit-config.yaml
+- repo: local
+ hooks:
+ - id: docker-clang-format
+ name: Docker Clang Format
+ language: docker_image
+ types:
+ - c++
+ entry: silkeh/clang:12
+ args:
+ - clang-format
+ - -style=file
+ - -i
+
- repo: local
hooks:
- id: check-style |
EXPERIMENTAL integration of clang-format hook into pre-commit.
NOT TO BE MERGED until after the 2.7 release.
At the moment this DRAFT PR is is to see what's involved and what the result looks like.
Using the same docker container as used for clang-tidy (silkeh/clang:12).