style: clang-format in pre-commit #3683

henryiii · 2022-02-03T18:07:25Z

Description

Full clang-format, in pre-commit.

Suggested changelog entry:

* Format the codebase with clang-format

Skylion007 · 2022-02-03T19:06:47Z

@henryiii I have a readability check that's been waiting on this clang-format. Let me add in it as well.

Skylion007 · 2022-02-03T19:36:20Z

include/pybind11/detail/common.h

-        std::is_integral<T>::value ? detail::log2(sizeof(T))*2 + std::is_unsigned<T>::value : 8 + (
-        std::is_same<T, double>::value ? 1 : std::is_same<T, long double>::value ? 2 : 0));
+    static constexpr int index
+        = std::is_same<T, bool>::value


yikes to some of this formatting...

I'm fine to change the format rules, especially if they make it more LLVM like. Autoformatting is not perfectly "automatic", a little fiddling can be done to get it to format nicely (like moving comments).

Skylion007 · 2022-02-03T19:38:53Z

include/pybind11/cast.h

 #include "detail/common.h"
 #include "detail/descr.h"
 #include "detail/type_caster_base.h"
 #include "detail/typeid.h"
+#include "pytypes.h"


This isn't right, we need to add special clang-format comments to fix this.

Skylion007 · 2022-02-03T19:58:45Z

This also broke a lot of clang-tidy comments.

henryiii · 2022-02-03T21:17:10Z

Looks like decltype(PyThread_create_key()) is an int for Python < 3.7, which is muddling with readability-qualified-auto. I'm impressed I have that showing up in my VIM. :)

henryiii · 2022-02-03T21:30:21Z

I've set the include order to be pybind11 first, detail/* second, other local includes third. The order shouldn't matter, we should IWYU, but for now, this hopefully will work.

henryiii · 2022-02-03T21:31:34Z

Ahh, NOLINTNEXTLINE should count logical lines. :'(

henryiii · 2022-02-03T21:36:58Z

$ brew install act
$ act -j clang-tidy

Very nice. :)

henryiii · 2022-02-03T21:54:16Z

include/pybind11/detail/common.h

-template<size_t ...S> struct make_index_sequence_impl <0, S...> { using type = index_sequence<S...>; };
-template<size_t N> using make_index_sequence = typename make_index_sequence_impl<N>::type;
+template <size_t...>
+struct index_sequence {};


These sorts of things would be a lot more readable in the new form with an extra blank line in-between.

rwgk · 2022-02-04T00:06:10Z

Quick question: when I tried this previously I had trouble with

/** \rst
\endrst */

comments getting garbled. I even wrote a small script to massage those, to not get messed up by clang-format.

But now it seems to be fine without doing anything special. (?)

Mostly curious: Does anyone know what changed?

rwgk · 2022-02-04T00:19:45Z

include/pybind11/pytypes.h

-        .. _PyMemoryView_FromMemory: https://docs.python.org/c-api/memoryview.html#c.PyMemoryView_FromMemory
-     \endrst */


clang-format still seems to be unaware of reStructuredText.

Previously the doc generation would fail after I ran clang-format (clang 12).

Are we just getting lucky now?

rwgk · 2022-02-04T01:03:28Z

I wrote an even simpler script to extract the \rst...\endrst blocks to a file,

ran it on current master (36813cf),
then again on this PR (e379d5d).

Here is the diff: rwgk/pybind11_scons@fa4ffb4

I think we need to curate the changed blocks, at least some, and protect them from clang-format.

I believe it's best to systematically protect all such \rst blocks, which is what the other script does. It's slightly annoying, but really only a few lines extra and easy to maintain.

rwgk · 2022-02-04T01:34:59Z

An alternative line of thought:

I believe clang-format getting into reflowing comments will be generally more annoying than helpful.
All I want it to do is warn about lines being longer than 99 chars, with the usual exceptions for (apparent) URLs and filenames.
Then leave it to users to fix manually.

henryiii · 2022-02-04T03:56:48Z

I'm quite okay to turn off comment formatting, I have it off for other areas (including CMake comments, maybe even in this repo). I'd rather have no comment formatting vs. having to watch out for all RST blocks being manually protected.

Can we keep the "nice" changes (like reflowing text to the right length) and use your script to recover the rst formatting, then just turn it off?

henryiii · 2022-02-04T03:57:31Z

(I'm also okay for comments to go over length, but I don't know how to turn that off, including in black)

rwgk · 2022-02-04T04:11:49Z

Can we keep the "nice" changes (like reflowing text to the right length) and use your script to recover the rst formatting, then just turn it off?

Sounds great, I'll try to work on it tomorrow morning.

(I'm also okay for comments to go over length, but I don't know how to turn that off, including in black)

Yeah, that's a minor loss. After we get this PR merged, we could maybe hook in a small script that flags long comments. I bet a page worth of Python will do most of the job, with some simple approach to allowing exceptions.

henryiii · 2022-02-04T04:45:15Z

I'm hoping to avoid too much customization, we are getting rid of our custom formatting script. :) I think we may still get line length warnings - and I'm really not too worried about comments. The thing I don't like is this:

AVeryLongLongLongClass a_very_long_long_long_varaible_or_other_expression(); // NOLINT

Turning into this:

AVeryLongLongLongClass a_very_long_long_long_varaible_or_other_expression();
// NOLINT

or this:

AVeryLongLongLongClass
    a_very_long_long_long_varaible_or_other_expression(); // NOLINT

But I think we are stuck with that. For new code, you usually can rework it to make it shorter, us a NOLINTNEXTLINE or similar construct, etc.

This has an interesting side-effect on comments for pragmas. Manually tweaked to something that looks ok and is not modified by clang-format anymore.

rwgk · 2022-02-05T00:40:34Z

Can we keep the "nice" changes (like reflowing text to the right length) and use your script to recover the rst formatting, then just turn it off?

Done. My tweaks are in commit 60b7eb4. Setting ReflowComments: false is in the follow-on commit 59572e6. I was surprised to see that turning off comment reflow changed how pragma comments are handled (clang-format wanted to add backslashes, which looks weird to me). I went through a couple iterations until I settled on what you see.

rwgk · 2022-02-05T01:12:52Z

I spent a few minutes glancing through the net diffs. It sure does odd things with comments:

I manually changed that block to this:

    void *ptr = nullptr; // Pointer to the underlying storage
    ssize_t itemsize = 0; // Size of individual items in bytes
    ssize_t size = 0; // Total number of entries
    std::string format; // For homogeneous buffers, this should be set to format_descriptor<T>::format()
    ssize_t ndim = 0; // Number of dimensions
    std::vector<ssize_t> shape; // Shape of the tensor (1 entry per dimension)
    std::vector<ssize_t> strides; // Number of bytes between adjacent entries (for each per dimension)
    bool readonly = false; // flag to indicate if the underlying storage may be written to

But then rerunning the pre-commit changed it straight back.

What I'd really want: enforce one space (or two?) exactly before //, but then pretend everything after that space doesn't exist, to not trigger wrapping of the code just because of the trailing comment.

The rest looks acceptable, although I cannot say that I like how it deals with m.def() and member .def(). But I'm OK sacrificing that for the bigger gain of not having to manually move whitespaces around.

henryiii · 2022-02-06T06:16:51Z

Okay, I'll see if we can handle those things better.

henryiii · 2022-02-06T19:32:36Z

Looks like it’s hard to improve WRT comments. What should be done is long trailing comments need to be placed above the line, not on the same one. We need some new blank lines separating sequences of definitions, too. Or we could turn up the line length a lot.

Skylion007 · 2022-02-06T20:02:18Z

include/pybind11/chrono.h

-        }
-        else return false;
+            msecs = microseconds(PyDateTime_TIME_GET_MICROSECOND(src.ptr()));
+        } else


Stuff like this is why I woould vote always endorcing braces.

Stuff like this is why I woould vote always endorcing braces.

Absolutely. (Missing braces have tripped me up many times.)

If you have code automatically formatted, then whitespace is always in sync with braces, so it doesn't bother me at all. I just completely ignore braces and pretend it's Python. :) Though I'm totally fine with requiring the braces, as I said, I ignore them anyway. ;)

@henryiii https://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html as an alternative

rwgk · 2022-02-07T16:13:30Z

What should be done is long trailing comments need to be placed above the line, not on the same one.

I think so, too.

With this in my mind: when developing pybind11 in the future, clang-format will always do one thing or another that doesn't look great. Then we will look and tweak to make it look ok before sending PRs for review.
Moving these comments around is basically retro-active tweaking. I.e. perfectly fine.

Related but separate note: when I worked on this previously I was organizing the work to have all manual changes before running clang-format. It's extra work, but it looks much better in the git history: it will be easy to see what was manually changed. If we notice problems later, we will more easily understand how they came about, and therefore feel more certain how to properly fix. We will also have complete certainty which issues were introduced by clang-format, and again be more certain about how to fix; and we have something to point to for filing clang-format bugs.

Once we agree here, I could work on splitting into two PRs (part 1: manual, part 2: exclusively automatic).

henryiii · 2022-02-07T17:03:30Z

I'd recommend always working on things like this in two commits. The first is the manual tweaking, and then the second is after the auto run. Then you can just keep force-pushing changes to the first comment & the followup auto-run.

So we need to add the always-braces rule, and maybe move or add whitespace around a few of the really egregious comments and bunches of previously one-line statements?

henryiii · 2022-02-07T17:18:31Z

Clang-format can't do this, actually, but readability-braces-around-statements for clang-tidy can.

rwgk · 2022-02-07T17:44:36Z

Clang-format can't do this, actually, but readability-braces-around-statements for clang-tidy can.

I'm surprised!
I'll try a separate PR (now) to see what happens if we add that clang-tidy option.

Informed by work under pybind#3683: pybind@60b7eb4 pybind@59572e6

@henryiii

) * Manual line breaks to pre-empt undesired `clang-format`ing. Informed by work under #3683: 60b7eb4 59572e6 * Manual curation of clang-format diffs involving source code comments. Very labor-intensive and dull. * Pulling .clang-format change from @henryiii's 9057962 * Adding commonly used .clang-format `CommentPragmas:` * Ensure short lambdas are allowed Co-authored-by: Aaron Gokaslan <[email protected]>

henryiii added 2 commits February 3, 2022 13:01

chore: add clang-format

8ee4eb3

style: run clang-format

53056b1

Skylion007 added 3 commits February 3, 2022 14:10

Add readability-qualified-auto to clang-tidy

287527f

Fix faulty double const additions

aab27e5

Use member initializer

a2bbae2

Skylion007 reviewed Feb 3, 2022

View reviewed changes

Improve readability using data()

d3746a9

fix: add explicit ordering to includes

9057962

fix: support Python < 3.6

63f0695

henryiii force-pushed the henryiii/style/clang-format branch from e248d94 to 63f0695 Compare February 3, 2022 21:23

fix: run clang-tidy action locally and fix remaining issues

e379d5d

henryiii commented Feb 3, 2022

View reviewed changes

rwgk reviewed Feb 4, 2022

View reviewed changes

Ralf W. Grosse-Kunstleve added 2 commits February 4, 2022 16:17

Manual rst-endrst fixups, based on rwgk/pybind11_scons@fa4ffb4

60b7eb4

ReflowComments: false

59572e6

This has an interesting side-effect on comments for pragmas. Manually tweaked to something that looks ok and is not modified by clang-format anymore.

henryiii mentioned this pull request Feb 6, 2022

Python 2 removal #3688

Merged

Skylion007 reviewed Feb 6, 2022

View reviewed changes

This was referenced Feb 8, 2022

clang-tidy readability-qualified-auto #3702

Merged

Use member initializer #3704

Merged

Manually applying two clang-format changes that need fix-ups for clang-tidy. #3705

Merged

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Feb 9, 2022

Manual line breaks to pre-empt undesired clang-formating.

b34b2ac

Informed by work under pybind#3683: pybind@60b7eb4 pybind@59572e6

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Feb 10, 2022

Manual line breaks to pre-empt undesired clang-formating.

58da37c

Informed by work under pybind#3683: pybind@60b7eb4 pybind@59572e6

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Feb 10, 2022

Manual line breaks to pre-empt undesired clang-formating.

e4e7dea

Informed by work under pybind#3683: pybind@60b7eb4 pybind@59572e6

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Feb 10, 2022

Manual line breaks to pre-empt undesired clang-formating.

d8ec2ee

Informed by work under pybind#3683: pybind@60b7eb4 pybind@59572e6

henryiii closed this Feb 10, 2022

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: clang-format in pre-commit #3683

style: clang-format in pre-commit #3683

henryiii commented Feb 3, 2022

Skylion007 commented Feb 3, 2022

Skylion007 Feb 3, 2022

henryiii Feb 3, 2022

Skylion007 Feb 3, 2022 •

edited

Loading

Skylion007 commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii Feb 3, 2022

rwgk commented Feb 4, 2022

rwgk Feb 4, 2022

rwgk commented Feb 4, 2022

rwgk commented Feb 4, 2022

henryiii commented Feb 4, 2022 •

edited

Loading

henryiii commented Feb 4, 2022

rwgk commented Feb 4, 2022

henryiii commented Feb 4, 2022

rwgk commented Feb 5, 2022

rwgk commented Feb 5, 2022

henryiii commented Feb 6, 2022

henryiii commented Feb 6, 2022

Skylion007 Feb 6, 2022

rwgk Feb 7, 2022

henryiii Feb 7, 2022

Skylion007 Feb 7, 2022

rwgk commented Feb 7, 2022 •

edited

Loading

henryiii commented Feb 7, 2022 •

edited

Loading

henryiii commented Feb 7, 2022

rwgk commented Feb 7, 2022

		.. _PyMemoryView_FromMemory: https://docs.python.org/c-api/memoryview.html#c.PyMemoryView_FromMemory
		\endrst */

style: clang-format in pre-commit #3683

style: clang-format in pre-commit #3683

Conversation

henryiii commented Feb 3, 2022

Description

Suggested changelog entry:

Skylion007 commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Skylion007 commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

henryiii commented Feb 3, 2022

Choose a reason for hiding this comment

rwgk commented Feb 4, 2022

Choose a reason for hiding this comment

rwgk commented Feb 4, 2022

rwgk commented Feb 4, 2022

henryiii commented Feb 4, 2022 • edited Loading

henryiii commented Feb 4, 2022

rwgk commented Feb 4, 2022

henryiii commented Feb 4, 2022

rwgk commented Feb 5, 2022

rwgk commented Feb 5, 2022

henryiii commented Feb 6, 2022

henryiii commented Feb 6, 2022

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

rwgk commented Feb 7, 2022 • edited Loading

henryiii commented Feb 7, 2022 • edited Loading

henryiii commented Feb 7, 2022

rwgk commented Feb 7, 2022

Skylion007 Feb 3, 2022 •

edited

Loading

henryiii commented Feb 4, 2022 •

edited

Loading

rwgk commented Feb 7, 2022 •

edited

Loading

henryiii commented Feb 7, 2022 •

edited

Loading