style: add clang-format file #2310

henryiii · 2020-07-20T18:54:50Z

This is an attempt at using clang-format for the C++ style. If this is accepted, then a) we can drop the old check_style.sh script (will be done in this PR), b) all open PR's are likely to need to run pre-commit to become compatible again before merging. Due to point b), we probably will want to wait till a specific point to merge this - so for now, this should be seen more as a discussion for the format style file we would like to adopt (if we go this way).

A few points about how I came up with the style:

It is based on LLVM as a start
I was trying to mimic the old style as closely as possible, rather than making ideal choices - maybe some should change.
- Quite a few places are inconsistent, so had to pick style over the other

And, about the style itself:

PyBind follows LLVM's int *x convention, vs. int* x - I've seen good arguments for the latter, though the former is (only) better if you are listing multiple items, which you should not do. We should verify we want to keep it
I've left the 2-space indent for public/private/protected.

AlignConsecutiveAssignments: true # Varies in the source, sometimes aligned, sometimes not
AlwaysBreakTemplateDeclarations: Yes # Varies in the source, but more often than not
BinPackArguments: false # Varies in the source, but this looks better, IMO
BinPackParameters: false
BreakBeforeBinaryOperators: All # Mostly/always followed
BreakConstructorInitializers: BeforeComma 
ColumnLimit: 99 # Wasn't always followed - matching flake8 here
IndentPPDirectives: AfterHash # Often was not indented the same, though - 2 spaces instead of 4
IndentWidth: 4
Language: Cpp
SpaceAfterCStyleCast: true # Varied a little bit, but this seemed to be the most common
# SpaceInEmptyBlock: true # too new, so { } will be changed to {}
Standard: Cpp11
TabWidth: 4

Edit: Ignore failures for now, this is for a discussion of the style, should have skipped CI.

Also, since this is a fairly large change, I think we should also move again with a check_style merged into pre-commit check so we can continue with adding GHA. We can also consider applying black at roughly the same time, assuming the format looks okay, when this PR is ready to be accepted?

bstaletic · 2020-07-21T08:40:56Z

My two cents: Formatting style doesn't really matter as long as it is consistent. At that point, just pick any style and move on. I only have one strong opinion. Do not indent with 2 spaces. Since that's not the case, I'm fine with whatever.

wjakob · 2020-07-21T08:51:23Z

Looks suprisingly good, I am impressed. Two adjustment requests:

BreakConstructorInitializers: BeforeColon
IndentCaseLabels: true

Is there some way to avoid the 2-space indentation of public/private/..? I don't think we previously did that in the codebase.

YannickJadoul

As noted before, I'm of course for coding conventions, but I'm not a huge fan of automatic formatting that don't allow for any exceptions. Pointed out a few.

But of course I'll still live if this gets merged ;-) Just wondering if it's really worth it.

YannickJadoul · 2020-07-21T13:44:17Z

include/pybind11/functional.h

+    using type          = std::function<Return(Args...)>;
+    using retval_type   = conditional_t<std::is_same<Return, void>::value, void_type, Return>;
+    using function_type = Return (*)(Args...);


AlignConsecutiveAssignments: true # Varies in the source, sometimes aligned, sometimes not

Ugh, really?

I mean... there's two types of consecutive assignments: one where you are assigning bunch of similar things (like enum values, or fields of a date/time struct), and another one where you just happen to assign two things consecutively.
Somehow, it feels exaggerated or even confusing in the latter case to align them?

If they are not related, you can add a line in between. These are all type settings, so I don't think it's that harmful to align them.

(I'm also fine not aligned, too - no strong opinions on style, really, just want consistency and some way to automate)

Yeah, OK, they're related (so probably extra lines are weird), but there's nothing parallel in form in the assignments (i.e., they're not all assigning 1, 2, 3, 4, ...).

But yes, maybe that's the disadvantage to the advantage of having everything automated...

include/pybind11/iostream.h

YannickJadoul · 2020-07-21T13:47:07Z

include/pybind11/stl_bind.h

-    template <typename T2> static std::false_type test_comparable(...);
-    template <typename T2> static std::true_type test_value(typename T2::value_type *);
-    template <typename T2> static std::false_type test_value(...);
-    template <typename T2> static std::true_type test_pair(typename T2::first_type *, typename T2::second_type *);
-    template <typename T2> static std::false_type test_pair(...);


Are these short helpers really not an exception to the rule? Do they need to take up twice as many lines?

The first one is really very long - it's a bit more readable when shortened in a normal width window. What needs to be done here is to add a blank line in between each item.

YannickJadoul · 2020-07-21T13:48:01Z

include/pybind11/stl_bind.h

+    cl.def(
+        "count",


Ugh? But but but ... "def count:" is nice if it's on one line, no? :-(

You can play with "priorities" of breaks and have a fine grained control over where clang-format inserts new lines. I played with these priorities once for two hours and didn't achieve what I want, but that's a different story.

Sure, yes. Just meant as another example where a human might have thought about this, and a formatter doesn't understand the semantics. pybind11's own docs suggest the previous style: https://pybind11.readthedocs.io/en/stable/classes.html#binding-lambda-functions

The problem with putting them on one line is what do you do with the next line? For example,

my_really_fantastic_and_great_object_of_some_sort.def("count", a_lambda_or_somethign_else_1, a_lambda_or_somethign_else_2 );

Gives you potentially very little space, and most editors won't get the spacing right for you when you are writing it (since it's a variable indent).

my_really_fantastic_and_great_object_of_some_sort.def("count", a_lambda_or_somethign_else_1, a_lambda_or_somethign_else_2 );

makes the "count" much harder to notice/read with the other items.

my_really_fantastic_and_great_object_of_some_sort.def( "count", a_lambda_or_somethign_else_1, a_lambda_or_somethign_else_2 );

is the only one that is consistent and readable if the arguments and/or object name are sometimes lengthy.

There are quite a few packing controls, though. I think I use the experimental auto packing flag somewhere else.

Yeah, sure, I can see that. But here it's just cl.def, short enough, which might influence judgment?

At any rate, I find it a weird inconsistency with the docs.

For what it is worth, I also prefer not breaking before the first argument if there's enough space, like in this case.

henryiii · 2020-07-21T14:11:55Z

don't allow for any exceptions

You can turn clang-format off if you want a specific style in a specific spot; there's nothing wrong with that. It then also gives the reader a signal that you really care about the exact formatting of a block of code. However, for a good format, I think it's often not really that helpful - once you get used to a style, deviations from it are usually not helpful. While they may seem great to the person writing them at the time, they generally just serve to confuse the person reading it because it is inconsistent. Remember you cannot communicate to C++ (or Python) via style, so it is entirely something you are trying to communicate to the programmer on top of whatever you are actually communicating to the program.

YannickJadoul · 2020-07-21T14:18:41Z

While they may seem great to the person writing them at the time, they generally just serve to confuse the person reading it because it is inconsistent.

That might be true, yes. I'm sure I'll adapt to it as well. Just putting this out there.

Remember you cannot communicate to C++ (or Python) via style, so it is entirely something you are trying to communicate to the programmer on top of whatever you are actually communicating to the program.

Exactly! But how can you communicate anything more through formatting if everything needs to follow the fixed rules? If I see template <...> on the same line as struct, I read: ah, right, this is just a short one-line helper-thing, rather than a full class.

But yeah, that's maybe too philosophical. Everyone else seems excited to get this in, so thanks for configuring all this! :-)

henryiii · 2020-07-21T14:33:25Z

right, this is just a short one-line helper-thing, rather than a full class.

But shouldn't that be communicated through the code itself, the name, or a comment? What happens if you have a conceptually "short one-line helper" that happens to be really long or a few more lines?

YannickJadoul · 2020-07-21T15:32:49Z

But shouldn't that be communicated through the code itself, the name, or a comment? What happens if you have a conceptually "short one-line helper" that happens to be really long or a few more lines?

Also, definitely, but if you can make it easier to immediately spot, just by the shape of the code... But anything's a habit, I suppose.

henryiii · 2020-07-27T04:09:07Z

I've removed the "application" commit and made this a manual hook only, so it can be applied if someone wants to, but will not be required everywhere (yet). I don't think it should be applied until we are in a fairly PR-free state - and maybe new code could be expected to pass it, etc.

rwgk · 2021-01-12T18:03:23Z

@henryiii pointed me here, thanks!

Merging this without hook (for now) would be great, then at least we can all run with the same config on new or modified code. Doing that for a while will give us experience what works well and what may need tweaking. At some opportune moment we could go through systematically, then activate the hooks.

How will this relate do clang-tidy as documented in the Clang-Tidy section in .github/CONTRIBUTING.md?

Things I'm also wondering about: 1. how exactly do I run clang-format manually on a specific file? 2. guidance/recommendations for using // NOLINT (I guess).
Would it make sense to explain this is a small section in .github/CONTRIBUTING.md?

EDIT:
Re. 1. above, it's simply clang-format --style=file -i my_c++_source_file. The command automatically searches for .clang-format up the directory hierarcy.
Re. 2. above, it's // clang-format on, // clang-format off, for sections of code. I didn't see an option to disable formatting for a single line.

henryiii · 2021-01-15T04:37:40Z

If this passes, I'm in favor of merging, though we should keep all whitespace change PRs separated, and list style-only changes in .git-blame-ignore-revs.

henryiii · 2021-01-15T04:38:23Z

Also happy to have someone add docs, points 1 and 2 are correct above. ;)

include/pybind11/iostream.h

rwgk · 2021-01-15T19:14:56Z

Also happy to have someone add docs, points 1 and 2 are correct above. ;)

I'd be happy to contribute a small section, although I'm still unclear about this question (above):

How will this relate do clang-tidy as documented in the Clang-Tidy section in .github/CONTRIBUTING.md?

Could you please give me a rough idea? I can work on polishing the wording.

henryiii · 2021-01-15T20:44:04Z

How will this relate do clang-tidy as documented in the Clang-Tidy section in .github/CONTRIBUTING.md?

These are two separate checks. The docs could probably sit side by side, due to the similarity in name, but they don't have anything else in common - clang-tidy has to know how to compile the code, clang-format does not, etc.

I would put a clear disclaimer that we are not reformatting old code with this style yet. Probably will happen right after #2445 , at a guess.

YannickJadoul reviewed Jul 21, 2020

View reviewed changes

henryiii force-pushed the ci/gha2 branch from bc3cec9 to 261e5f7 Compare July 21, 2020 13:54

henryiii force-pushed the ci/gha2 branch from 261e5f7 to 5521210 Compare July 21, 2020 14:25

henryiii added the on hold label Jul 23, 2020

henryiii force-pushed the ci/gha2 branch from 5521210 to fa9d67a Compare July 27, 2020 04:06

henryiii force-pushed the ci/gha2 branch from fa9d67a to a7218af Compare July 27, 2020 04:19

henryiii closed this Aug 20, 2020

henryiii reopened this Aug 20, 2020

rwgk added a commit to rwgk/pybind11 that referenced this pull request Jan 12, 2021

Copying .clang-format from PR pybind#2310 (a7218af).

c1aaa98

henryiii force-pushed the ci/gha2 branch 2 times, most recently from 1bd8b1d to d6dc7ce Compare January 13, 2021 19:41

rwgk added a commit to rwgk/pybind11 that referenced this pull request Jan 14, 2021

Copying .clang-format from PR pybind#2310 (a7218af).

35d69bc

henryiii force-pushed the ci/gha2 branch from d6dc7ce to a35c7c2 Compare January 15, 2021 04:36

rwgk reviewed Jan 15, 2021

View reviewed changes

include/pybind11/iostream.h Show resolved Hide resolved

rwgk added a commit to rwgk/pybind11 that referenced this pull request Jan 18, 2021

Copying .clang-format from PR pybind#2310 (a7218af).

529e58f

rwgk mentioned this pull request Jan 18, 2021

Adding a Clang-Format section. #2803

Merged

style: adding clang-format as manual hook

4ab5492

henryiii force-pushed the ci/gha2 branch from a35c7c2 to 4ab5492 Compare January 18, 2021 19:37

docs: adding a Clang-Format section. (#2803)

e8a829e

henryiii marked this pull request as ready for review January 18, 2021 19:39

henryiii added this to the v2.6.2 milestone Jan 18, 2021

henryiii closed this Jan 19, 2021

henryiii reopened this Jan 19, 2021

henryiii changed the title ~~style: Use clang-format~~ style: add clang-format file Jan 19, 2021

henryiii removed the on hold label Jan 19, 2021

henryiii merged commit 2db0264 into master Jan 20, 2021

henryiii deleted the ci/gha2 branch January 20, 2021 00:10

github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 20, 2021

henryiii added a commit that referenced this pull request Jan 20, 2021

docs: fix missing line from #2310

5b43ac4

henryiii removed the needs changelog Possibly needs a changelog entry label Jan 20, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2310

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: add clang-format file #2310

style: add clang-format file #2310

henryiii commented Jul 20, 2020 •

edited

Loading

bstaletic commented Jul 21, 2020

wjakob commented Jul 21, 2020

YannickJadoul left a comment

YannickJadoul Jul 21, 2020

henryiii Jul 21, 2020

henryiii Jul 21, 2020

YannickJadoul Jul 21, 2020

YannickJadoul Jul 21, 2020

henryiii Jul 21, 2020

YannickJadoul Jul 21, 2020

bstaletic Jul 21, 2020

YannickJadoul Jul 21, 2020

henryiii Jul 21, 2020

YannickJadoul Jul 21, 2020

bstaletic Jul 21, 2020

henryiii commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

henryiii commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

henryiii commented Jul 27, 2020

rwgk commented Jan 12, 2021 •

edited

Loading

henryiii commented Jan 15, 2021

henryiii commented Jan 15, 2021

rwgk commented Jan 15, 2021

henryiii commented Jan 15, 2021

style: add clang-format file #2310

style: add clang-format file #2310

Conversation

henryiii commented Jul 20, 2020 • edited Loading

bstaletic commented Jul 21, 2020

wjakob commented Jul 21, 2020

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

henryiii commented Jul 21, 2020

YannickJadoul commented Jul 21, 2020

henryiii commented Jul 27, 2020

rwgk commented Jan 12, 2021 • edited Loading

henryiii commented Jan 15, 2021

henryiii commented Jan 15, 2021

rwgk commented Jan 15, 2021

henryiii commented Jan 15, 2021

henryiii commented Jul 20, 2020 •

edited

Loading

rwgk commented Jan 12, 2021 •

edited

Loading