ENH: Add clang-format file #19754
ENH: Add clang-format file #19754
Conversation
|
Nice, browsing the options a bit, would |
5aa1593 to
a993beb
Compare
|
Already uncovered some errors by sorting the includes 👎
Yes. We might also want to change the line length for C++ at some point, although I would prefer not to. There are a few other changes we might want to make. |
Our includes are terrible :( (and I am happy to admit, that I am not making it better). There is some maze where include order matters due to INTERNAL BUILD and maybe other things. Or where files just work because they include a header that incidentally includes something they need... It would be extremely nice to clean it up a bit, I feel I am missing some best practices/intuition, though. |
numpy/core/src/multiarray/convert.c
Outdated
| @@ -368,8 +369,7 @@ PyArray_FillWithScalar(PyArrayObject *arr, PyObject *obj) | ||
| * the element in that array instead. | ||
| */ | ||
| if (PyArray_DESCR(arr)->type_num == NPY_OBJECT && | ||
| !(PyArray_Check(obj) && | ||
| PyArray_NDIM((PyArrayObject *)obj) == 0)) { | ||
| !(PyArray_Check(obj) && PyArray_NDIM((PyArrayObject *)obj) == 0)) { | ||
| value = (char *)&obj; | ||
|
|
There was a problem hiding this comment.
Need an indent of the continued condition expression. I think the BSD style has that.
a993beb to
6a3b501
Compare
|
Made a couple of other changes. Waiting for criticisms :) |
numpy/core/src/multiarray/convert.c
Outdated
| || (PyArray_IS_F_CONTIGUOUS(self) && (order == NPY_FORTRANORDER))) { | ||
| ret = PyBytes_FromStringAndSize(PyArray_DATA(self), (Py_ssize_t) numbytes); | ||
| if ((PyArray_IS_C_CONTIGUOUS(self) && (order == NPY_CORDER)) || | ||
| (PyArray_IS_F_CONTIGUOUS(self) && (order == NPY_FORTRANORDER))) { |
There was a problem hiding this comment.
I don't see any option to force greater indentation in the if statement condition continuation.
There was a problem hiding this comment.
Hmmm, maybe AlignAfterOpenBracket: DontAlign, although likely it will have its own issues? Or does that not affect conditionals?
4e8286e to
4adb838
Compare
The google style sorts the headers in groups by level, so it doesn't sort them all together. It also looks like One potential problem is the clang-format version. There are features in version 12 that would be useful. Hmm, looks like it is available in focal (updates) and I have it in fedora 34, so maybe we can go that way. What I'd like to do is forbid one line enums, but we don't have that many of them and that choice is probably arguable. |
Some code styles require that header files be stand alone, that is, that they include everything needed to compile. |
|
clang-format currently puts spaces around |
4adb838 to
3864bbc
Compare
3864bbc to
ad0e0e2
Compare
|
I think this is as good as it can get for now. Using it requires |
numpy/core/src/common/array_assign.h
Outdated
| @@ -1,6 +1,8 @@ | ||
| #ifndef _NPY_PRIVATE__ARRAY_ASSIGN_H_ | ||
| #define _NPY_PRIVATE__ARRAY_ASSIGN_H_ | ||
|
|
||
| #include "numpy/arrayobject.h" | ||
|
|
There was a problem hiding this comment.
Why the extra include?
It was to make the include order safe. However, it turns out that .clang-format provides a mechanism for automatically grouping and ordering includes, so I am going to formalize our include ordering.
EDIT: Once I figure out what it is :)
There was a problem hiding this comment.
As far as I can tell this include is not moved, it is new to the file.
There was a problem hiding this comment.
this include is not moved
True, it happened to sort after the needed numpy/* includes, so didn't cause a problem, but that was an accident. I'm going the put the numpy/* includes into their own group so they always come first and then we won't need to worry.
Could we do like python: add a linter and have it report errors only on code that is part of the PR (without taking a stand on the files you have already processed)? |
Maybe, there are several integrations that I haven't explored yet. I do know that it can be run on line ranges. |
ac15f3e to
b985d59
Compare
b985d59 to
b639cb4
Compare
There are several actions available in the github Marketplace, I haven't looked closely at them. Some automatically reformat files on merge, others look to reformat differences on pushes to PRs. Not sure what would be best. If I could, I'd reformat all the C files first, that would also allow working out any kinks found in the process. But I know that there is resistance to massive reformatting at the point, although I did it in the past. The template files are also likely to pose difficulties, although I haven't tried yet. On the plus side, most of the changes seem to involve indentation and spacing around operators, and we are already pretty good on that score. Note that at this time all the binary operators get spaces, including |
b639cb4 to
da29682
Compare
da29682 to
7ad94ed
Compare
|
Close/reopen |
7ad94ed to
3b6040c
Compare
b4162ae to
082646f
Compare
Clang-format can be used to reformat C/C++ code for codestyle fixups. The `.clang-format` file here implements the NumPy codestyle document as much as possible. Co-authored-by: Paul Ganssle <paul@ganssle.io>
082646f to
92a1067
Compare
|
I've removed the example so that future changes after reformatting can go into separate PRs. |
|
FYI I don't recall how I came up with that original file, but presumably I did it myself since I felt comfortable releasing it the first time someone asked, so if that licensing question is still active you have my blessing (not sure if it even qualifies for copyright anyway). |
Clang-format can be used to reformat C/C++ code for codestyle fixups. The
.clang-formatfile here implements the NumPy codestyle document as much as possible. The initial version here is taken from @pganssle's file athttps://gist.github.com/pganssle/0e3a5f828b4d07d79447f6ced8e7e4db, he is listed as co-author.
I've included a formatted version of convert.c for before/after comparison, it can be removed before merging.
Closes #19363.