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

ClangFormat the C code #4770

Merged
merged 3 commits into from
Jan 4, 2021
Merged

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Jul 9, 2020

Fixes #4493.

I found a "clang-format style that approximates Python's PEP 7":

(The zoneinfo repo was the reference implementation for zoneinfo in the stdlib (PEP 615), and is now a backport for Python 3.6+.)

This style has also been used elsewhere, as is or as the basis:

Including CPython's new PEG generator (written by Guido and two others), which is the same as the zoneinfo one but with ColumnLimit increased from 79 to 95:

Here's how it looks applied to Pillow:

clang-format -i `git ls-tree -r HEAD --name-only | grep "\.[ch]$"`
  1. The first commit is the same config file as in zoneinfo
  2. The second formats the codebase with the config
  3. The third increases ColumnLimit from 79 to 88, to match Black
  4. The fourth formats again with the 88 limit

I suggest we use this, and welcome feedback and any further tweaks.

Before merge, to make the history cleaner, I'll squash the config to a single commit, and the formatting to another single commit.

Also this will need adding to the CI.

@wiredfool
Copy link
Member

wiredfool commented Jul 9, 2020

I really dislike this form of function definition, especially since almost all other forms of braces are inline with the end of the starting line:

static PyObject*
_putdata(ImagingObject* self, PyObject* args)
{

The rest looks pretty reasonable. I think this is forcing braces around single line blocks (if/while), which is good. If it's not, then I'd say that it should.

@nulano
Copy link
Contributor

nulano commented Jul 9, 2020

I think this ignores the following part of PEP 7:

  • Breaking long lines: if you can, break after commas in the outermost argument list. Always indent continuation lines appropriately, e.g.:
PyErr_Format(PyExc_TypeError,
            "cannot create '%.100s' instances",
            type->tp_name);

I think this is also a requirement in Python code in Black.

@hugovk
Copy link
Member Author

hugovk commented Jul 10, 2020

I really dislike this form of function definition, especially since almost all other forms of braces are inline with the end of the starting line:

static PyObject*
_putdata(ImagingObject* self, PyObject* args)
{

This config sets BreakBeforeBraces: Stroustrup, overriding the Google value of BreakBeforeBraces: Attach.

The options (docs with examples):

  • Attach: Always attach braces to surrounding context.
  • Linux: Like Attach, but break before braces on function, namespace and class definitions.
  • Stroustrup: Like Attach, but break before function definitions.
  • Allman: Always break before braces.
  • GNU: Always break before braces and add an extra level of indentation to braces of control statements, not to those of class, function or other definitions.

Attach is the only one not to break before that brace:

static PyObject *
_putdata(ImagingObject *self, PyObject *args) {

I've changed config to BreakBeforeBraces: Attach in ba22421 and formatted with it in 4466b71. Is it better?

@hugovk
Copy link
Member Author

hugovk commented Jul 10, 2020

I think this is forcing braces around single line blocks (if/while), which is good. If it's not, then I'd say that it should.

I was hoping it would, to prevent bugs like #4492, but alas it's out of scope for clang-format (https://stackoverflow.com/a/36871872/724176).

@radarhere fixed up the existing code in #4618.

@hugovk
Copy link
Member Author

hugovk commented Jul 10, 2020

I think this ignores the following part of PEP 7:

  • Breaking long lines: if you can, break after commas in the outermost argument list. Always indent continuation lines appropriately, e.g.:
PyErr_Format(PyExc_TypeError,
            "cannot create '%.100s' instances",
            type->tp_name);

I think this is also a requirement in Python code in Black.

Looks like this can be achieved with (https://stackoverflow.com/a/47097008/724176):

AlignAfterOpenBracket: AlwaysBreak
BinPackArguments: false
BinPackParameters: false

Docs: https://clang.llvm.org/docs/ClangFormatStyleOptions.html
Config: f714356
Formatted: a48cc59

@hugovk
Copy link
Member Author

hugovk commented Aug 13, 2020

I've rebased, squashed the style modifications to a single commit, and reformatted in a single commit.

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

Let's do this after the 2020-04-01 release (#4354), when the PR list will be at a local minimum and to avoid merge conflicts.

Maybe after 8.0.0 on 2020-10-15 (#4764)?


/* -------------------------------------------------------------------- */
/* OBJECT ADMINISTRATION */
/* -------------------------------------------------------------------- */

typedef struct {
PyObject_HEAD
Imaging image;
PyObject_HEAD Imaging image;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. Perhaps the following should get formatted properly?

Suggested change
PyObject_HEAD Imaging image;
PyObject_HEAD;
Imaging image;

This change can be done in all files with a simple grep.

Copy link
Member

Choose a reason for hiding this comment

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

Adding in the semicolon causes syntax errors on Windows (AppVeyor and GHA).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah great, leave it to MSVC to apply arbitrary restrictions from a standard it doesn't even fully support yet.

After a bit of effort, I have found the following hack which works with both MSVC and GCC 4.1 and has no effect on structure size (see https://godbolt.org/z/76W7f4):

Suggested change
PyObject_HEAD Imaging image;
PyObject_HEAD int:0;
Imaging image;

The extra code can be hidden in a define such as #define PILLOW_HEAD PyObject_HEAD int:0 and used similarly to my original comment. However, I'm not sure whether it is worth the extra effort.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this if possible, PyObject_HEAD is the standard way to do it, and hiding it in a macro is additional complication that doesn’t make sense except for formatting.

@radarhere
Copy link
Member

I don't have strong feelings about this.
We're coming up on a release. Is it a good time to merge?

@wiredfool
Copy link
Member

Let’s merge after release. Fewer potential merge errors and rebase required then.

@radarhere radarhere merged commit d374015 into python-pillow:master Jan 4, 2021
@hugovk hugovk deleted the clang-format-pganssle branch January 4, 2021 12:35
@hugovk hugovk mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClangFormat the C code
4 participants