Apply clang-format #6323

JohannesLorenz · 2022-03-05T16:43:18Z

This code here should be the same as in #5948 . Asking for final review.

A good "playground" might be src/gui/editors/PianoRoll.cpp (>5000 lines).

Note: The review can also end with "Let's better not take clang-format", that is also OK.

LmmsBot · 2022-03-05T16:57:21Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.185+g9c49e78c4-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.185+g9c49e78c4-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.185+g9c49e78c4-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.3.0-alpha.1.185+g9c49e78c4-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f8fdeafb-b9cd-4229-8c48-73bb7be732de/artifacts/0/lmms-1.3.0-alpha.1.185+g9c49e78c4-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16614?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/511c5315-e9f8-46c1-8175-fac414decf64/artifacts/0/lmms-1.3.0-alpha.1.185+g9c49e78c4-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16610?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/a66c729e-287e-4c12-a6b1-53fb597396ad/artifacts/0/lmms-1.3.0-alpha.1.185+g9c49e78c4-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16612?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1avf797ms690wya6/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43398151"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/5shx66gtuim4e77t/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43398151"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/5f139266-c005-4dde-9854-6365b97e0966/artifacts/0/lmms-1.3.0-alpha.1.185+g9c49e78c4-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16611?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "25bc6f40eba4b2c8ce611bcec1beea642c0fed80"}

JohannesLorenz · 2022-03-05T20:04:13Z

I added a github action (see build number 15).

include/ActionGroup.h

plugins/stk/mallets/mallets.cpp

M374LX · 2022-03-05T21:19:02Z

include/BandLimitedWave.h

-		if (table % 2 == 0) {
-			return m_data[TLENS[table] + ph];
-		} else {
+		if (table % 2 == 0) { return m_data[TLENS[table] + ph]; }


The if is on a single line, but the else is not. It looks inconsistent.

Putting the else completely into one line seems impossible (possibly a bug or limitation to AllowShortIfStatementsOnASingleLine). As soon as I turn this else into an if, it works.

I can only change it to

if (table % 2 == 0) { return m_data[TLENS[table] + ph]; } else { return m_data3[TLENS[table] + ph]; }

Is that better or worse?

This looks better:

if (table % 2 == 0) { return m_data[TLENS[table] + ph]; } else { return m_data3[TLENS[table] + ph]; }

A ternary if is even better:
return (table % 2 == 0) ? m_data[TLENS[table] + ph] : m_data3[TLENS[table] + ph];

I know, but the coding rules said If the block can fit on one line, it is acceptable. I think we must live with the bad else.

Using a ternary is nice, but clang-format can't do that.

include/BandLimitedWave.h

JohannesLorenz · 2022-03-06T16:17:27Z

src/gui/editors/PianoRoll.cpp

-		{
-			note->setLength(bound);
-		}
+		if (constrainMax ? note->length() > bound : note->length() < bound) { note->setLength(bound); }


It's in 1 line now and fits the 120 columns, so the tool does what we requested, but I'm not sure if it got more readable.

allejok96 · 2022-03-11T21:38:38Z

General impressions:

Long lines are alright, they are still readable because the formatting is very consistent.
Is it possible to prevent single line function definitions? See end of piano roll.
Agreed that the if else inconsistency is annoying
Long strings that have been split across multiple lines by not using comma seems to remain untouched
Some switch statements are slightly obscured (more than usual) when a multi-line case is preceded by a one-line case, perhaps just a break. See piano roll paint event.
Otherwise LGTM

JohannesLorenz · 2022-03-18T22:24:57Z

@allejok96 Thanks for your review.

Is it possible to prevent single line function definitions? See end of piano roll.

Yes, I added AllowShortFunctionsOnASingleLine: InlineOnly to .clang-format. It fixes then mentioned definitions in PianoRoll.cpp.

Some switch statements are slightly obscured (more than usual) when a multi-line case is preceded by a one-line case, perhaps just a break. See piano roll paint event.

Obscured in what way? Can you please point out which exact line you mean, or paste a code snipped?

allejok96 · 2022-03-20T18:20:36Z

It might be very rare but in this case it feels like the break gets hidden (from PianoRoll's paint event)

switch (prKeyOrder[topNote])
{
    case PR_WHITE_KEY_SMALL:
    case PR_WHITE_KEY_BIG: break;
    case PR_BLACK_KEY:
        // draw extra white key
        drawKey(topKey + 1, grid_line_y - m_keyLineHeight);
}

JohannesLorenz · 2022-04-06T18:25:07Z

It might be very rare but in this case it feels like the break gets hidden

Indeed, I did not even see this when scrolling through it. Patching clang-format like

-AllowShortCaseLabelsOnASingleLine: true
+AllowShortCaseLabelsOnASingleLine: false

fixed it. Of course then, some switch statements get larger. Imagine one with 50 or 100 cases where every case only returns. On the other hand, the hidden break is really ugly and could be bug prone.

I'm rather for disabling one-line-cases, any opinions?

JohannesLorenz · 2022-04-30T10:36:57Z

any opinions?

@allejok96 , what do you think?

allejok96 · 2022-04-30T12:55:00Z

Glancing over the use of switch statements there are some places where you've got lot of things crammed into one line:

case 1: w = (float)fabs(2.0f*(float)sin(fmod(0.5f*ph,TwoPi)))-1.f; break; //sine^2

Or you do this horrible whitespace thing:

case Base::UserVST          : loc = ConfigManager::inst()->userVstDir(); break;

I'd rather see each statement on a separate line.

Imagine one with 50 or 100 cases

In theory this would be a problem, but currently the only such switch statement (>20 cases) in non-third-party code is in PianoView.cpp. And it will still be possible to do manual formatting, right?

So my opinion is 👍 @JohannesLorenz

This adds `.clang-format` and `.clang-tidy` files to check the code (partially) against the [LMMS coding conventions](https://github.com/LMMS/lmms/wiki/Coding-conventions).

JohannesLorenz · 2022-04-30T16:03:19Z

Thanks @allejok96 !

JohannesLorenz mentioned this pull request Mar 5, 2022

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

M374LX reviewed Mar 5, 2022

View reviewed changes

include/ActionGroup.h Outdated Show resolved Hide resolved

M374LX reviewed Mar 5, 2022

View reviewed changes

plugins/stk/mallets/mallets.cpp Outdated Show resolved Hide resolved

M374LX reviewed Mar 5, 2022

View reviewed changes

plugins/stk/mallets/mallets.cpp Outdated Show resolved Hide resolved

M374LX reviewed Mar 5, 2022

View reviewed changes

JohannesLorenz commented Mar 5, 2022

View reviewed changes

include/BandLimitedWave.h Outdated Show resolved Hide resolved

JohannesLorenz commented Mar 6, 2022

View reviewed changes

Add .clang-format and .clang-tidy

25bc6f4

This adds `.clang-format` and `.clang-tidy` files to check the code (partially) against the [LMMS coding conventions](https://github.com/LMMS/lmms/wiki/Coding-conventions).

JohannesLorenz force-pushed the clang-format branch from f5c4d51 to 25bc6f4 Compare April 30, 2022 14:53

JohannesLorenz merged commit 3964c53 into LMMS:master Apr 30, 2022

This was referenced May 4, 2022

Clang tools testing #5948

Closed

Add .clang-format and .clang-tidy #4690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply clang-format #6323

Apply clang-format #6323

JohannesLorenz commented Mar 5, 2022

LmmsBot commented Mar 5, 2022 •

edited

Loading

Linux

Windows

macOS

JohannesLorenz commented Mar 5, 2022

M374LX Mar 5, 2022

JohannesLorenz Mar 5, 2022

M374LX Mar 5, 2022 •

edited

Loading

JohannesLorenz Mar 5, 2022

JohannesLorenz Mar 6, 2022

allejok96 commented Mar 11, 2022

JohannesLorenz commented Mar 18, 2022

allejok96 commented Mar 20, 2022

JohannesLorenz commented Apr 6, 2022

JohannesLorenz commented Apr 30, 2022

allejok96 commented Apr 30, 2022

JohannesLorenz commented Apr 30, 2022

Apply clang-format #6323

Apply clang-format #6323

Conversation

JohannesLorenz commented Mar 5, 2022

LmmsBot commented Mar 5, 2022 • edited Loading

Linux

Windows

macOS

JohannesLorenz commented Mar 5, 2022

M374LX Mar 5, 2022

Choose a reason for hiding this comment

JohannesLorenz Mar 5, 2022

Choose a reason for hiding this comment

M374LX Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

JohannesLorenz Mar 5, 2022

Choose a reason for hiding this comment

JohannesLorenz Mar 6, 2022

Choose a reason for hiding this comment

allejok96 commented Mar 11, 2022

JohannesLorenz commented Mar 18, 2022

allejok96 commented Mar 20, 2022

JohannesLorenz commented Apr 6, 2022

JohannesLorenz commented Apr 30, 2022

allejok96 commented Apr 30, 2022

JohannesLorenz commented Apr 30, 2022

LmmsBot commented Mar 5, 2022 •

edited

Loading

M374LX Mar 5, 2022 •

edited

Loading