ENH: Add clang-format file #19754

charris · 2021-08-25T19:30:27Z

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. The initial version here is taken from @pganssle's file at
https://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.

seberg · 2021-08-25T19:50:58Z

Nice, browsing the options a bit, would ContinuationIndentWidth: 8 be closer to current usage?

charris · 2021-08-25T20:31:58Z

Already uncovered some errors by sorting the includes 👎

would ContinuationIndentWidth: 8 be closer to current usage?

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.

seberg · 2021-08-25T20:34:38Z

Already uncovered some errors by sorting the includes -1

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.

charris · 2021-08-25T20:45:37Z

numpy/core/src/multiarray/convert.c

@@ -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;



Need an indent of the continued condition expression. I think the BSD style has that.

charris · 2021-08-25T21:40:53Z

Made a couple of other changes. Waiting for criticisms :)

charris · 2021-08-25T21:50:12Z

numpy/core/src/multiarray/convert.c

-        || (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))) {


I don't see any option to force greater indentation in the if statement condition continuation.

Hmmm, maybe AlignAfterOpenBracket: DontAlign, although likely it will have its own issues? Or does that not affect conditionals?

charris · 2021-08-26T02:10:19Z

Our includes are terrible

The google style sorts the headers in groups by level, so it doesn't sort them all together. It also looks like #define breaks them into groups, but I am not sure of that.

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.

charris · 2021-08-26T02:14:21Z

I feel I am missing some best practices/intuition, though.

Some code styles require that header files be stand alone, that is, that they include everything needed to compile.

charris · 2021-08-26T02:53:49Z

clang-format currently puts spaces around * and / even though that isn't required by the google style guide.

charris · 2021-08-26T20:03:14Z

I think this is as good as it can get for now. Using it requires clang-format version 12. Should the three reformatted files be included?

mattip · 2021-09-01T13:55:23Z

numpy/core/src/common/array_assign.h

@@ -1,6 +1,8 @@
 #ifndef _NPY_PRIVATE__ARRAY_ASSIGN_H_
 #define _NPY_PRIVATE__ARRAY_ASSIGN_H_

+#include "numpy/arrayobject.h"
+


Why the extra include?

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 :)

As far as I can tell this include is not moved, it is new to the file.

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.

mattip · 2021-09-01T13:58:52Z

Should the three reformatted files be included?

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)?

charris · 2021-09-01T14:12:12Z

Could we do like python

Maybe, there are several integrations that I haven't explored yet. I do know that it can be run on line ranges.

charris · 2021-09-01T19:49:05Z

add a linter and have it report errors only on code that is part of the PR

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 * and /, which is annoying but can be lived with.

charris · 2021-09-03T15:21:23Z

Close/reopen

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>

charris · 2021-09-03T16:58:12Z

I've removed the example so that future changes after reformatting can go into separate PRs.

pganssle · 2021-09-03T23:44:14Z

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).

charris added the 01 - Enhancement label Aug 25, 2021

github-actions bot added the 25 - WIP label Aug 25, 2021

charris force-pushed the add-clang-format-file branch from 5aa1593 to a993beb Compare August 25, 2021 20:10

charris commented Aug 25, 2021

View reviewed changes

charris force-pushed the add-clang-format-file branch from a993beb to 6a3b501 Compare August 25, 2021 21:39

charris commented Aug 25, 2021

View reviewed changes

charris force-pushed the add-clang-format-file branch 2 times, most recently from 4e8286e to 4adb838 Compare August 26, 2021 00:31

charris force-pushed the add-clang-format-file branch from 4adb838 to 3864bbc Compare August 26, 2021 03:43

charris removed the 25 - WIP label Aug 26, 2021

charris force-pushed the add-clang-format-file branch from 3864bbc to ad0e0e2 Compare August 26, 2021 20:01

github-actions bot added the 25 - WIP label Aug 26, 2021

charris changed the title ~~WIP, ENH: Add clang-format file~~ ENH: Add clang-format file Aug 26, 2021

charris added a commit to charris/numpy that referenced this pull request Aug 26, 2021

DOC: Add a release note for numpygh-19754.

ac15f3e

charris removed the 25 - WIP label Aug 27, 2021

mattip reviewed Sep 1, 2021

View reviewed changes

charris added the 25 - WIP label Sep 1, 2021

charris added a commit to charris/numpy that referenced this pull request Sep 1, 2021

DOC: Add a release note for numpygh-19754.

b985d59

charris force-pushed the add-clang-format-file branch from ac15f3e to b985d59 Compare September 1, 2021 19:02

charris added a commit to charris/numpy that referenced this pull request Sep 1, 2021

DOC: Add a release note for numpygh-19754.

b639cb4

charris force-pushed the add-clang-format-file branch from b985d59 to b639cb4 Compare September 1, 2021 19:22

charris added a commit to charris/numpy that referenced this pull request Sep 1, 2021

DOC: Add a release note for numpygh-19754.

da29682

charris force-pushed the add-clang-format-file branch from b639cb4 to da29682 Compare September 1, 2021 21:34

charris removed the 25 - WIP label Sep 2, 2021

charris added a commit to charris/numpy that referenced this pull request Sep 3, 2021

DOC: Add a release note for numpygh-19754.

7ad94ed

charris force-pushed the add-clang-format-file branch from da29682 to 7ad94ed Compare September 3, 2021 15:19

charris closed this Sep 3, 2021

charris reopened this Sep 3, 2021

charris added a commit to charris/numpy that referenced this pull request Sep 3, 2021

DOC: Add a release note for numpygh-19754.

3b6040c

charris force-pushed the add-clang-format-file branch from 7ad94ed to 3b6040c Compare September 3, 2021 15:29

charris added a commit to charris/numpy that referenced this pull request Sep 3, 2021

DOC: Add a release note for numpygh-19754.

eb100b4

charris force-pushed the add-clang-format-file branch 2 times, most recently from b4162ae to 082646f Compare September 3, 2021 15:38

ENH: Add a clang-format file.

92a1067

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>

charris force-pushed the add-clang-format-file branch from 082646f to 92a1067 Compare September 3, 2021 16:56

charris merged commit 644c55e into numpy:main Sep 3, 2021

charris deleted the add-clang-format-file branch September 3, 2021 18:26

tgross35 mentioned this pull request May 2, 2022

DOC: Style guide could indicate formatters / pre-commit hook #21424

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add clang-format file #19754

ENH: Add clang-format file #19754

charris commented Aug 25, 2021 •

edited

seberg commented Aug 25, 2021

charris commented Aug 25, 2021

seberg commented Aug 25, 2021

charris Aug 25, 2021

charris commented Aug 25, 2021

charris Aug 25, 2021

seberg Aug 25, 2021

charris commented Aug 26, 2021

charris commented Aug 26, 2021

charris commented Aug 26, 2021

charris commented Aug 26, 2021

mattip Sep 1, 2021

charris Sep 1, 2021 •

edited

mattip Sep 1, 2021

charris Sep 1, 2021

mattip commented Sep 1, 2021

charris commented Sep 1, 2021

charris commented Sep 1, 2021 •

edited

charris commented Sep 3, 2021

charris commented Sep 3, 2021

pganssle commented Sep 3, 2021

ENH: Add clang-format file #19754

ENH: Add clang-format file #19754

Conversation

charris commented Aug 25, 2021 • edited

seberg commented Aug 25, 2021

charris commented Aug 25, 2021

seberg commented Aug 25, 2021

charris Aug 25, 2021

Choose a reason for hiding this comment

charris commented Aug 25, 2021

charris Aug 25, 2021

Choose a reason for hiding this comment

seberg Aug 25, 2021

Choose a reason for hiding this comment

charris commented Aug 26, 2021

charris commented Aug 26, 2021

charris commented Aug 26, 2021

charris commented Aug 26, 2021

mattip Sep 1, 2021

Choose a reason for hiding this comment

charris Sep 1, 2021 • edited

Choose a reason for hiding this comment

mattip Sep 1, 2021

Choose a reason for hiding this comment

charris Sep 1, 2021

Choose a reason for hiding this comment

mattip commented Sep 1, 2021

charris commented Sep 1, 2021

charris commented Sep 1, 2021 • edited

charris commented Sep 3, 2021

charris commented Sep 3, 2021

pganssle commented Sep 3, 2021

charris commented Aug 25, 2021 •

edited

charris Sep 1, 2021 •

edited

charris commented Sep 1, 2021 •

edited