Add a .clang-format file #940

LeonarddeR · 2023-10-06T18:38:36Z

I invested an hour or two writing down a .clang-format that more or less follows the code style that is now common in OSARA.
@jcsteh What would you like? Should I add a reformatted version of one file as well? Then you and probably others can comment on changed lines you dislike.
Before merge we can revert these changes, then after merge I can provide a pr that should be with the whole code base formatted, including a .git-blame-ignore-revs. We should merge that using rebase and merge or a regular merge commit, not squashing.

Fixes #923

AppVeyorBot · 2023-10-06T18:44:47Z

Build succeeded! Build osara pr940-1178,7c33d940 completed (commit 7c33d940dd by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1178,7c33d940.dmg

AppVeyorBot · 2023-10-07T11:49:46Z

Build succeeded! Build osara pr940-1180,ac9dc06b completed (commit ac9dc06b9e by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1180,ac9dc06b.dmg

AppVeyorBot · 2023-10-07T11:57:48Z

Build succeeded! Build osara pr940-1181,dba8a65f completed (commit dba8a65fdd by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1181,dba8a65f.dmg

jcsteh

Thanks very much for working on this.

I think it might be worth seeing the diff for reaper_osara.cpp. It's the biggest and oldest file, so this will give us a good idea of how much churn there will be and how we might minimise it.

.clang-format

jcsteh · 2023-10-07T13:40:27Z

.clang-format

+# Do not add a space after the template keyword.
+SpaceAfterTemplateKeyword: false
+# Sort includes (CaseSensitive, CaseInsensitive, or None).
+SortIncludes: CaseInsensitive


I think there are some edge cases where include order matters; e.g. see the inclusion of osara.h in fxChain.cpp and others. Will this break that?

I'll have a look and will act accordingly.

Right, we definitely want to leave include ordering alone.

LeonarddeR · 2023-10-07T16:18:26Z

I addressed your comments and added one commit with the whole code base formatted, so you can have a look on what it will do.

AppVeyorBot · 2023-10-07T16:36:28Z

Build succeeded! Build osara pr940-1182,c89b942e completed (commit c89b942ec6 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1182,c89b942e.dmg

.clang-format

AppVeyorBot · 2023-10-10T05:43:17Z

Build failed! Build osara pr940-1183,a69381b2 failed (commit a69381b2dd by @LeonarddeR)

jcsteh · 2023-10-10T05:50:35Z

Not a strange question at all. The reason is that xgettext doesn't support some of the things we need such as .rc files and string lists (translateFirstString, etc.).

LeonarddeR · 2023-10-10T06:01:34Z

Actually with my last change, I think we are now pretty safe. WhitespaceSensitiveMacros will just leave alone the translate and translate_plural calls.

src/uia.cpp

.clang-format

LeonarddeR · 2023-10-10T17:28:11Z

I just took a different tack and took the Microsoft style as a basis. This seems to be going a lot better.

LeonarddeR · 2023-10-10T17:51:47Z

O wait, Microsoft doesn't have a maximum column count it seems. Things go wild again if we limit the amount of columns on 80.
I'm a bit surprised that clang format makes things so difficult for us.

AppVeyorBot · 2023-10-10T19:28:27Z

Build succeeded! Build osara pr940-1184,95c113b0 completed (commit 95c113b0e1 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1184,95c113b0.dmg

AppVeyorBot · 2023-10-10T19:38:05Z

Build succeeded! Build osara pr940-1185,32beb394 completed (commit 32beb39469 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1185,32beb394.dmg

jcsteh · 2023-10-11T22:43:59Z

src/envelopeCommands.cpp

+					// Translators: A shape for an envelope point.
+					translate("square"),
+							// Translators: A shape for an envelope point.
+							translate("slow start/end"),


Wo. What on earth is going on here? It seems to keep adding continuation indents even though each translate call is closed. I wonder if WhitespaceSensitiveMacros is interfering here somehow?

It does seem that AlignAfterOpenBracket: DontAlign makes this worse. But it's definitely very confused. If I add random indentation to these lines, it preserves that even with AlignAfterOpenBracket: BlockIndent. So I guess the whitespace sensitivity applies to the line before the macro, not just inside the macro, which is very unfortunate.

I think I'm going to need to fix makePot... somehow.

I'm not sure. There's also some black magic going on in uia.cpp I referenced earlier, which I think is related. It really looks like we are missing an alignment setting somewhere, but I'm pretty sure we have covered them all.

I'm starting to regret this all.

I really do appreciate you working on this. Even if we end up shelving it for a while, we can always come back to it.

I'll look at fixing makePot at some point to cope with multi-line calls. I have some ideas on how it might be done and that's probably a good idea for other reasons anyway.

Losing translateFirstString is a bit of a shame, but I guess that was always a bit sketchy anyway, and not having to worry about code formatting eventually (which is a total waste of people's time) probably outweighs that loss.

Sure. I will mark this as draft for now.

AppVeyorBot · 2023-10-18T08:20:51Z

Build failed! Build osara pr940-1199,8d680658 failed (commit 8d680658fe by @LeonarddeR)

jcsteh · 2023-10-18T08:35:06Z

I wonder if we can get around the include problem with tightly scoped clang-format ignores. It'd be nice if we didn't have to worry about include ordering except in very specific cases.

AppVeyorBot · 2023-10-18T09:15:23Z

Build succeeded! Build osara pr940-1201,0cd70440 completed (commit 0cd7044014 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1201,0cd70440.dmg

AppVeyorBot · 2023-10-18T09:22:34Z

Build succeeded! Build osara pr940-1202,bcd1d6fd completed (commit bcd1d6fd03 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1202,bcd1d6fd.dmg

src/controlSurface.cpp

jcsteh · 2023-10-18T09:38:02Z

src/reaper_osara.cpp

-				wParam != VK_F6 && wParam != 'B' &&
-				wParam != VK_TAB && wParam != VK_DOWN &&
-				wParam != VK_UP && wParam != VK_SPACE)) {
+	if (!isKeyDown || code != HC_ACTION || (wParam != VK_APPS && wParam != VK_CONTROL && wParam != VK_F10 && wParam != VK_RETURN && wParam != VK_F6 && wParam != 'B' && wParam != VK_TAB && wParam != VK_DOWN && wParam != VK_UP && wParam != VK_SPACE)) {


What the actual hell? What happened to the maximum line length? This actually makes this much harder to read.

Wow this is extremely weird indeed. It might have to do with a penalty that is part of the Google style and set to a pretty big value.
May be Google is really not a good starting point for us. Probably go with Microsoft as a basis instead. Note however that Microsoft has no line length enforcement. My feeling says that 80 might be a bit too low for us. If we go for something around 100, the uia issue with the ternary operator would also be gone. Not an ideal solution, though.

I actually fixed continuation indent and now it seems to break up properly again.

It now adds breaks again, but insists to add an extra space to the second line:

if (!isKeyDown || code != HC_ACTION || (wParam != VK_APPS && wParam != VK_CONTROL && wParam != VK_F10 && wParam != VK_RETURN && wParam != VK_F6 && wParam != 'B' && wParam != VK_TAB && wParam != VK_DOWN && wParam != VK_UP && wParam != VK_SPACE)) {

I'm utterly confused!

jcsteh · 2023-10-18T09:39:24Z

src/reaper_osara.cpp

-		translate("phase normal"));
+	outputMessage(
+		isTrackPhaseInverted(track) ? translate("phase inverted")
+																: translate("phase normal")


Definitely something wacky going on with ternary operators. It's like it's trying to align horizontally instead of using continuation indentation.

jcsteh · 2023-10-18T09:44:33Z

src/reaper_osara.cpp

-	{MIDI_EDITOR_SECTION, {DEFACCEL, _t("OSARA: Select all notes with the same pitch starting in time selection")}, "OSARA_SELSAMEPITCHTIMESEL", cmdMidiSelectSamePitchStartingInTimeSelection},
-	{ MIDI_EDITOR_SECTION, {DEFACCEL, _t("OSARA: Mute next message from OSARA")}, "OSARA_ME_MUTENEXTMESSAGE", cmdMuteNextMessage},
+	{MAIN_SECTION,
+	 {DEFACCEL, _t("OSARA: Move to next item (leaving other items selected)")},


And here we don't seem to be using block indent, despite AlignAfterOpenBracket. IN fact, we end up using a space here instead of a tab, probably because of the single brace. I don't get it. We should be using block indent or continuation indent, never horizontal align.

jcsteh · 2023-10-18T09:47:38Z

Oh. clang-format has this to say about AlignAfterOpenBracket: BlockIndent:

This currently only applies to braced initializer lists (when Cpp11BracedListStyle is true) and parentheses.

I mean... I would have thought if conditions count as parentheses, but apparently not. But it does explain why it doesn't impact braces.

LeonarddeR · 2023-10-18T10:28:40Z

I wonder if we can get around the include problem with tightly scoped clang-format ignores. It'd be nice if we didn't have to worry about include ordering except in very specific cases.

I will look into that later today/this week.

LeonarddeR · 2023-10-18T17:51:19Z

Ii now chose the Microsoft style as a basis, which has a 120 character column limit. At least that saves us some very ugly code breaking.

AppVeyorBot · 2023-10-18T17:56:13Z

Build succeeded! Build osara pr940-1208,b8ca0d3a completed (commit b8ca0d3a77 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1208,b8ca0d3a.dmg

jcsteh · 2023-10-18T21:55:39Z

Which breaks were bothering you with the Google style? If they're a real problem, can we just increase the max line length there?

LeonarddeR · 2023-10-20T13:39:39Z

We're now back at Google, and indeed, it formats some stuff slightly nicer than Microsoft.

AppVeyorBot · 2023-10-20T13:45:08Z

Build succeeded! Build osara pr940-1209,bb85eb17 completed (commit bb85eb1752 by @LeonarddeR)
Downloads:

Windows

Mac

installer/osara_pr940-1209,bb85eb17.dmg

Timtam · 2024-05-24T12:19:59Z

Just coming here because i'm noticing how long it takes to fix things like indentation all the time. I noticed that you seem to struggle quite alot with getting clang-format to work. Is there any specific reason why you absolutely want to go with this over any other, newer solution like a prettier or something similar? AFAIK clang-format works, but is pretty old and most likely outdated compare to more modern tools. We already have Python toolchain with scons, we could go for a Python-based implementation of a similar tool instead, maybe that would work better? Just a thought though.

jcsteh · 2024-05-24T12:23:37Z

I'm curious as to what you're suggesting is a more modern tool. As I understand it, clang-format is the industry standard these days.

The main problem is that OSARA's existing formatting isn't really compatible with any of the more prevalent C++ coding styles. We could just reformat the whole code base without caring, but that isn't great for anyone already familiar with contributing to the project. Also, there are sections of the code that have been formatted the way they have for very specific reasons, so we need to consider the trade-offs there.

Timtam · 2024-05-24T12:41:26Z

I'm curious as to what you're suggesting is a more modern tool. As I understand it, clang-format is the industry standard these days.

I'm not too deep into "the industry" anymore. Last time I was working for a tech company they were doing all the formatting with a pre-commit hook through the IDE, namely IntelliJ. Dunno if that one uses clang-format under the hood, but I kinda doubt it. I know other people who do all the formatting in IDE too, tools like clang-format, prettier and such usually just are fallbacks there, but obviously that is nowhere near the entire picture.

We could just reformat the whole code base without caring, but that isn't great for anyone already familiar with contributing to the project.

IMO the advantage of not having to care about the entire formatting at all is way, way more important than keeping the current formatting in tact. I watched Scott and Jen navigate the codebase multiple times by now, kinda all that matters is that you can count the braces and parens there, indentation is there but usually gets skipped anyway, and nooone is entirely sure how to indent properly anyway in specific cases like line breaks within a function call. IMO the actual way of formatting isn't as important as having the ability to just write down your code without having to care about indents and stuff, hit a button before committing and just ignore code formatting at all. That is basically what all my sighted colleagues do too from what I can tell. Noone nowadays cares about counting tabs and breaking lines correctly.

Also, there are sections of the code that have been formatted the way they have for very specific reasons, so we need to consider the trade-offs there.

Just put them onto a ignore list and don't format them at all? I know, sounds easier than it probably is, and I know you discussed that already. Could you give me a rundown why exactly that doesn't work properly with clang-format? I think I remember something along the lines of clang-format messes other things up if you do that? That might be one reason why it might be worth investigating other tools to see if they handle this case better.

jcsteh · 2024-05-24T12:56:09Z

I totally hear you on the advantages of reformatting the whole code base and just not caring about it from there. That's certainly been life changing in my work at Mozilla, for example. However, there are still trade-offs when figuring out what style you want, etc. Ideally, you don't want every single line of the code base to be rewritten during the initial reformat. Some of the formatting we were seeing with some of this work was really hard to read for some cases.

It doesn't help that our translation code is currently pretty brittle and relies on regular expressions, so it gets rather unhappy if things break across lines in ways it doesn't expect. We can't use xgettext because it doesn't support .rc files as far as I know, among other things.

Ignoring things works fine in clang-format. But then you don't get the advantages of having it consistent in that section of the code. This is totally a case of me wanting to have my cake and eat it too, but consider the lines we have in our command registration tables, just as an example:

{MAIN_SECTION, {{0, 0, 40285}, nullptr}, nullptr, cmdGoToNextTrack}, // Track: Go to next track

Because that line is long, any formatter is going to split it across multiple lines. You could end up with something like this:

{
  MAIN_SECTION,
  {{0, 0, 40285}, nullptr},
  nullptr,
  cmdGoToNextTrack
}, // Track: Go to next track

So now the registration for every command is 6 lines instead of 1. Given that this table is already 215 lines long, that means it would now be about 1290 lines long. This is just one example that comes to the top of my mind.

jcsteh · 2024-05-24T12:57:22Z

There's also the fact that this would break all existing open pull requests, some of which we can't merge yet for various reasons.

jcsteh reviewed Oct 7, 2023

View reviewed changes

LeonarddeR force-pushed the clangFormat branch from db16358 to d977560 Compare October 7, 2023 16:15

LeonarddeR force-pushed the clangFormat branch from d977560 to 4b8be4e Compare October 7, 2023 16:30

jcsteh reviewed Oct 9, 2023

View reviewed changes

.clang-format Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

LeonarddeR force-pushed the clangFormat branch from e687389 to e688156 Compare October 10, 2023 05:37

This comment was marked as resolved.

Sign in to view

LeonarddeR force-pushed the clangFormat branch from e688156 to 337fe25 Compare October 10, 2023 05:48

LeonarddeR force-pushed the clangFormat branch 2 times, most recently from d0d6a8a to 4f6442a Compare October 10, 2023 05:59

LeonarddeR commented Oct 10, 2023

View reviewed changes

src/uia.cpp Outdated Show resolved Hide resolved

jcsteh reviewed Oct 10, 2023

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

LeonarddeR force-pushed the clangFormat branch from 6a8e24d to 6568611 Compare October 10, 2023 17:27

LeonarddeR force-pushed the clangFormat branch from 6568611 to fafb5fc Compare October 10, 2023 19:22

LeonarddeR force-pushed the clangFormat branch from fafb5fc to 296103f Compare October 10, 2023 19:32

jcsteh reviewed Oct 11, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

LeonarddeR force-pushed the clangFormat branch from 8af40cc to d41c6ea Compare October 18, 2023 08:17

LeonarddeR mentioned this pull request Oct 18, 2023

makeConfigRc: support whitespace after opening and before closing parens #946

Merged

LeonarddeR force-pushed the clangFormat branch from d41c6ea to f162ea4 Compare October 18, 2023 09:09

jcsteh reviewed Oct 18, 2023

View reviewed changes

src/controlSurface.cpp Outdated Show resolved Hide resolved

jcsteh reviewed Oct 18, 2023

View reviewed changes

Add clang-format to .gitattributes

5bbd682

LeonarddeR force-pushed the clangFormat branch from d60e139 to ad08462 Compare October 18, 2023 17:50

LeonarddeR added 2 commits October 20, 2023 15:38

Add .clang-format

c9b9b5e

Format code base

3e50e18

LeonarddeR force-pushed the clangFormat branch from ad08462 to 3e50e18 Compare October 20, 2023 13:38

jcsteh force-pushed the master branch from 04733ad to 25178a3 Compare February 13, 2024 13:47

jcsteh mentioned this pull request May 14, 2024

Implement tempo and time signature reporting #1073

Merged

Add a .clang-format file #940

Are you sure you want to change the base?

Add a .clang-format file #940

Conversation

LeonarddeR commented Oct 6, 2023 • edited

AppVeyorBot commented Oct 6, 2023

AppVeyorBot commented Oct 7, 2023

AppVeyorBot commented Oct 7, 2023

jcsteh 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

LeonarddeR commented Oct 7, 2023

AppVeyorBot commented Oct 7, 2023

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

AppVeyorBot commented Oct 10, 2023

jcsteh commented Oct 10, 2023

LeonarddeR commented Oct 10, 2023

LeonarddeR commented Oct 10, 2023

LeonarddeR commented Oct 10, 2023 • edited

AppVeyorBot commented Oct 10, 2023

AppVeyorBot commented Oct 10, 2023

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

This comment was marked as resolved.

AppVeyorBot commented Oct 18, 2023

jcsteh commented Oct 18, 2023

AppVeyorBot commented Oct 18, 2023

AppVeyorBot commented Oct 18, 2023

Choose a reason for hiding this comment

LeonarddeR Oct 18, 2023 • edited

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

jcsteh commented Oct 18, 2023

LeonarddeR commented Oct 18, 2023

LeonarddeR commented Oct 18, 2023

AppVeyorBot commented Oct 18, 2023

jcsteh commented Oct 18, 2023

LeonarddeR commented Oct 20, 2023

AppVeyorBot commented Oct 20, 2023

Timtam commented May 24, 2024

jcsteh commented May 24, 2024

Timtam commented May 24, 2024

jcsteh commented May 24, 2024

jcsteh commented May 24, 2024

LeonarddeR commented Oct 6, 2023 •

edited

LeonarddeR commented Oct 10, 2023 •

edited

LeonarddeR Oct 18, 2023 •

edited