Clang tools testing #5948

JohannesLorenz · 2021-03-13T14:22:14Z

This is not intended to be merged (I'll do a new branch for the official). The intention is to

See if you're happy with clang's changes (comment the code/conversation if unsatisfied, please)
Find out how difficult it gets to rebase other branches on this branch

Rebasing your branch on this might work like this for single-commit branches:

clang-format -style=file -i $(git ls-files '*.cpp' '*.h')
git commit -am 'fixup'
git rebase -i HEAD~2, Chooese f for the second commit, accept.
git rebase clang-formatted

This is a testing brach for #4690 .

LmmsBot · 2021-03-13T14:44:51Z

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

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.82%2Bga6a09454f-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.82%2Bga6a09454f-mingw-win64.exe (build link)

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.82%2Bga6a0945-linux-x86_64.AppImage (build link)

macOS

macOS : lmms-1.3.0-alpha.1.82%2Bga6a09454f-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14454-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bga6a09454f-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14454?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14452-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bga6a09454f-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14452?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14453-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bga6a0945-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14453?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14456-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bga6a09454f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14456?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "c7424137f6f6d25aab1b7f905f9d20c44df62abf"}

src/core/lv2/Lv2Manager.cpp

src/gui/FileBrowser.cpp

Veratil

Just some glances around and comments on format

.clang-format

src/gui/editors/PianoRoll.cpp

IanCaio · 2021-03-14T14:04:11Z

Is it easy to change the clang format to use Type* foo; instead of Type *foo;? I'm personally fine with either, but I think most people voted on the former at Discord and we were trying to use it lately. If it's not an easy change I'm for just using Type *foo; to make things easier!

Preparation for clang-format. It will shuffle the includes, and this preparation will prevent errors from that step.

This enables files for checking against the [LMMS coding conventions](https://github.com/LMMS/lmms/wiki/Coding-conventions). There is no strategy for automatic testing yet.

This commit was created by solely running ``` clang-format -style=file -i $(git ls-files '*.cpp' '*.h') git commit -a ``` No functional changes.

JohannesLorenz · 2021-04-03T19:39:51Z

No matter what I configure (different break styles, different columnlimit), I always get

        auto controllerType = m_controllerConnection
-                       ? m_controllerConnection->getController()->type()
-                       : Controller::DummyController;
-       bool skipMidiController = Engine::getSong()->isSavingProject()
-                                                         && Engine::getSong()->getSaveOptions().discardMIDIConnections.value();
-       if (m_controllerConnection && controllerType != Controller::DummyController
-               && !(skipMidiController && controllerType == Controller::MidiController))
+               ? m_controllerConnection->getController()->type()
+               : Controller::DummyController;
+       bool skipMidiController = Engine::getSong()->isSavingProject() && Engine::getSong()->getSaveOptions().discardMIDIConnections.value();
+       if (m_controllerConnection && controllerType != Controller::DummyController && !(skipMidiController && controllerType == Controller::MidiController))
        {

~~That's really bad. I'm clueless how to fix this.~~ fixed

Veratil · 2021-04-18T19:29:39Z

To fix the bool and if statements we can try:

BreakBeforeBinaryOperators: NonAssignment

For the ternary operator:

BreakBeforeTernaryOperators: true

JohannesLorenz · 2021-04-23T18:33:12Z

@Veratil Thanks for the hint! That worked in some lines, but not in the marked line.

sziegler103

I just scrolled to a random spot to have a look. Looks a lot cleaner from the little I saw.

sziegler103 · 2021-06-09T05:58:32Z

plugins/DualFilter/DualFilterControlDialog.cpp

 #include "gui_templates.h"

-#define makeknob( name, x, y, model, label, hint, unit ) 	\
-	Knob * name = new Knob( knobBright_26, this); 			\


Just a heads up, any tabs formatting line-continuation characters in multi-line #define's to be in a single "column" are reduced to being only a single space after the line. I'm ambivalent to either style but wanted to bring it up.

plugins/DualFilter/DualFilterControlDialog.cpp

plugins/DualFilter/DualFilterControls.cpp

src/core/Track.cpp

src/gui/PianoView.cpp

Spekular · 2021-06-21T12:00:24Z

Have we ever done a poll on bracket style? I feel like it would make sense to include that in this if it's going to be changed.

I think this covers the common styles, correct me if something's missing:

// Heart
if (gum)
{
    chew(gum);
}
else
{
    doTheOtherThing();
}

// Rocket
if (gum) {
    chew(gum);
}
else {
    doTheOtherThing();
}

// Eye
if (gum) {
    chew(gum);
} else {
    doTheOtherThing();
}

JohannesLorenz · 2021-07-11T18:57:28Z

Looks like 👁️ is preferred.

@Spekular can you please fill out

BraceWrapping:
  AfterClass: true
  AfterControlStatement: true
  AfterEnum: true
  AfterFunction: true
  AfterNamespace: true
  AfterStruct: true
  AfterUnion: true
  AfterExternBlock: true
  BeforeCatch: true
  BeforeElse: true
  IndentBraces: false
  SplitEmptyFunction: true
  SplitEmptyRecord: true
  SplitEmptyNamespace: true

And reply how you think it should be, in your opinion?

JohannesLorenz · 2021-07-11T19:09:39Z

First of all, many thanks for the feedback on the PR.

Many of the comments can be resolved by using ColumnLimit: 120. So I will change that and append a commit with the changes, unless anyone says "no" in the next days. This will also make our CTORs look like

classname::classname()
    : obj1(x1)
    , obj2(x2)
    , obj3(x3)

which has been accepted (4 times) on Discord.

If they are smaller than ColumnLimit (120), it is packed into one line. If not, it remains unchanged.

Spekular · 2021-07-19T21:58:41Z

Looks like 👁️ is preferred.

@Spekular can you please fill out [...] And reply how you think it should be, in your opinion?

BraceWrapping:
AfterClass: true // why is this inverted to the rest?
AfterControlStatement: Never
AfterEnum: false
AfterFunction: false
AfterNamespace: false // unsure here, example shows no indentation in which case wrap may be good for visibility
AfterStruct: false
AfterUnion: false
AfterExternBlock: false // again, example shows no indentation here when false is set...
BeforeCatch: false
BeforeElse: false // to match vote above, I prefer true
IndentBraces: false
SplitEmptyFunction: true // shouldn't apply if AfterFunction is false
SplitEmptyRecord: true // see above
SplitEmptyNamespace: true // see above
// Additionally:
BeforeWhile: false
BeforeLambdaBody: false

My only concern with these settings is that in some of the examples disabling wrapping seems to disable indentation inside the brackets, or there's no indent in the first place. I'm using https://clang.llvm.org/docs/ClangFormatStyleOptions.html for reference.

AfterExternBlock:

true:
extern "C"
{
  int foo();
}

false:
extern "C" {
int foo();
}

AfterNamespace

true:
namespace
{
int foo();
int bar();
}

false:
namespace {
int foo();
int bar();
}

If these examples are the only options I would say true (wrapping the braces) is the least bad choice, assuming there's no way to get unwrapped braces with indent.

JohannesLorenz · 2021-07-24T09:28:34Z

AfterClass: true // why is this inverted to the rest?

Agreed, I set this to false, too. It looks strange to me if class braces are not wrapped, but enum braces are.

If these examples are the only options

That seems true for extern, and not for namespace, I can confirm that.

I would say true (wrapping the braces) is the least bad choice, assuming there's no way to get unwrapped braces with indent

In many cases, namespace and extern wrap the whole file. In this case, I think indent is overkill. Do you really want to indent the whole file (except license) for every class (namespace) ~~and every plugin (extern)~~ (extern is not wrapped around the whole plugin, only around the descriptor)? Which means I would use false in these 2 cases, too. git grep -A 5 extern on the current code also shows that most extern code is not indented. I hope setting them to false (like you filled it out) is OK for everyone.

SplitEmptyFunction: true // shouldn't apply if AfterFunction is false
SplitEmptyRecord: true // see above
SplitEmptyNamespace: true // see above

Yes, tested, this has no effect on the whole source.

Summary: I take it like you filled it out, except that I set it to false for classes, too.

JohannesLorenz · 2021-07-24T09:54:29Z

Note: I only ran clang-format for the first pass. I'll run clang-tidy if the changes here are accepted by everyone.

Note to self: You need to run clang-tidy -fix -config-file=.clang-tidy <FILENAME> -- -Iinclude -Igcc/{src,} -I/usr/include/qt/{QtCore,QtWidgets,QtGui,QtXml,}, followed by a further clang-format run.

Spekular · 2021-07-24T17:28:08Z

AfterClass: true // why is this inverted to the rest?

Agreed, I set this to false, too. It looks strange to me if class braces are not wrapped, but enum braces are.

What I mean is, in the documentation it looks like "true" provides unwrapped braces and "false" provides wrapped ones, as opposed to all other settings. I still want classes to look like this:

class a {
    // content
}

As for namespace and extern, I'll trust your judgement on those.

JohannesLorenz · 2021-07-24T19:05:51Z

What I mean is, in the documentation it looks like "true" provides unwrapped braces and "false" provides wrapped ones, as opposed to all other settings.

Oh, that's a documentation error at their example for AfterClass (the other examples are correct). I just tried both bool values, and it's exactly the opposite of their example. So I'll leave it as false.

JohannesLorenz · 2021-07-25T10:50:33Z

All comments that marked negative aspects of clang-format are now resolved. I'd like to invite everyone to check if you are now OK with the code and vote:

🚀 I have no remarks, we should take it
👀 I have remarks, but if they are fixed we can take it
❤️ We should not take clang-format at all, it does too many things that we don't like, it's too much automation

I'm not sad if anyone votes for ❤️ . Please simply vote what you think applies most.

plugins/Amplifier/Amplifier.cpp

PhysSong · 2022-02-05T06:38:38Z

plugins/vst_base/RemoteVstPlugin.cpp


+	if (!chunky) delete[] data;
+	delete[](sBank*) pBank;


Affected by llvm/llvm-project#53576

Grepping our whole code base, it's only 6 occurrences in the mentioned file, so I think we can do this inconsistent for now and possibly fix it after reorg.

JohannesLorenz · 2022-05-04T18:08:55Z

Deprecated by #6323 (and merged there).

JohannesLorenz force-pushed the clang-tools-testing branch from 0c66a64 to 551c818 Compare March 13, 2021 15:19