clang-format

[reneme]

I think we all kind of agree that we want clang-format (#424, #2037 (comment), #2946 (comment)). Now with the 3.0.0 tag being the "tip of master": Let's have another go at this? 😀

It apparently even supports Whitesmiths for some time (albeit I'm not sure how closely that resembles the current style). Frankly, when accepting the hazzle of a reformat, I guess moving to some more "commonly used style" would also make sense. I got used to Whitesmiths now, but I certainly remember having a fairly hard time parsing it at the beginnning. 😅

In any case: Having a CI job that ensures proper coding style from then on is probably also a good idea.

[randombit]

Yeah this should happen. Whatever we do doesn't have to match the existing style perfectly or anything. I'd say aim to get to 90% to the existing format, just so the initial diff is not completely enormous, and then start enforcing formatting in CI, etc. Then we can start tweaking options to make the format more compact.

[reneme]

I'd say aim to get to 90% to the existing format

After a bit of toying around with this, I'm quite pessimistic that this will ever converge to something without an enormous diff. 😞

Also, I feel, we should not try to iteratively change things in the clang-format configuration along the way. Each of those changes will again lead to large diffs and a barrier for back-porting.

I would suggest we open a Draft PR where we try to converge on a style configuration by force-pushing iterations. And once we've converged, let's try to stick with what we defined.

Here's a first suggestion as a starting point:

Language: Cpp

BasedOnStyle: Microsoft

IncludeBlocks: Regroup
IncludeCategories:
  - Regex:    '^<[^(.h)]+>'
    Priority: 1
  - Regex:    '^<botan/[^/]+\.h>'
    Priority: 2
  - Regex:    '^<botan/internal/[^/]+\.h>'
    Priority: 3
IncludeIsMainRegex: '$'
AlignEscapedNewlines: Left

PointerAlignment: Left
ReferenceAlignment: Left
QualifierAlignment: Left

BreakConstructorInitializers: BeforeComma
BreakInheritanceList: BeforeComma
ColumnLimit: 100
AllowShortBlocksOnASingleLine: true
AllowShortFunctionsOnASingleLine: Inline
IndentWidth: 3
IndentPPDirectives: BeforeHash
FixNamespaceComments: true
SeparateDefinitionBlocks: Always
KeepEmptyLinesAtTheStartOfBlocks: false

[reneme]

By the way (from our internal experience with clang-format): The formatting output doesn't seem to be 100% compatible between versions. I.e. if we were to enforce the formatting in CI, we'd likely force everyone to use the same version locally.

This proved to be feasible and we don't see it as a show stopper. But it is an inconvenience that we keep in mind.

[lieser]

Note that for checking the formatting in the CI we internally use the https://github.com/Sarcasm/run-clang-format helper.

[randombit]

Each of those changes will again lead to large diffs and a barrier for back-porting.

This isn't actually an issue because no matter what, release-2 is going to be formatted differently from release-3, and we never do backports within a release branch. If there is a bug in 3.2, and we fix it in 3.3, we're not going to backport that onto a 3.2 patch release; upgrade instead.

That said since it seems like clang-format can't actually start us out with something close to what we have and we're looking at a huge diff no matter what, we might as well try to get something close in the first pass.

[randombit]

Opened a proposed format + formatting script in #3502

I'm not entirely happy with it, some parts clang-format definitely gets wrong and I can't figure out how to get it to do the right thing, but overall I can live with it.

Currently that PR doesn't apply the changes. Run make fmt to get them. Also the script supports checking the formatting , but there isn't a CI job added yet. and a CI job that runs the script and fails if the formatting is not correct.

There are some further initial manual changes that I will make first to improve what clang-format spits out, along the lines of #3507, so that the final commit applying clang-format is purely that.

[reneme]

One thing to keep in mind: Currently, the examples have their own . .clang-format` file. This should be replaced with the central one.

[reneme]

Are there any open issues with clang-format at this point? :)

[randombit]

I need to mess with the config a bit more, but basically no. This will probably merge this week.

[randombit]

Blocker: currently the Windows amalgamation build is failing, unclear what the problem is.

[randombit]

I really have no idea what is happening here

C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um\wincrypt.h(995): error C3646: 'Type': unknown override specifier
C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um\wincrypt.h(996): error C3646: 'Subtype': unknown override specifier

https://github.com/randombit/botan/actions/runs/5079590438/jobs/9125517442?pr=3502

[reneme]

I could try locally in about an hour. Did some "workaround #define" go the way of the dodo?

[reneme]

Blocker: currently the Windows amalgamation build is failing, unclear what the problem is.

Fixed. windows.h must be included before wincrypt.h, otherwise: 💥 I pushed the patch, let's see what CI thinks.

[randombit]

#3502 has merged

[reneme]

🎉