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

JohannesLorenz · 2018-11-01T12:23:40Z

Don't merge yet, needs discussion, see below.

The following patch provides style files for ~2/3 of the LMMS coding conventions (~4/5 if clang-tidy will accept a PR I'll need to create).
clang-format and clang-tidy provide functionality to find style issues (the former for whitespace, the latter for code) and to fix them automatically.

The following works as an example (assuming you're in the main LMMS folder and use a build dir build). For best results, use what make VERBOSE=1 spits out.

# clang-format -style=file <filename>
clang-format -style=file src/core/AutomatableModel.cpp
# clang-tidy -config= <filename> -- <compile-flags>
clang-tidy -config-file=.clang-tidy src/core/AutomatableModel.cpp -- -Iinclude -Ibuild/{src,} -I/usr/include/qt/{QtCore,QtWidgets,QtGui,QtXml,}

QtCreator:
Full support for clang-format and clang-tidy 😄

clang-tidy:
- Analyze -> Clang-Tidy and Clazy
- Select custom style, click manage, duplicate existing one, select Clang-Tidy tab, choose to read style from file
- Select the files that you want to analyze (selecting all takes a long time!), click "Analyze"
clang-format
- Enable Help -> About Plugins -> C++ -> Beautifier
- Restart
- Tools -> Options -> Beautifier, do your settings, but especially, go to the "Clang Format" tab, use predefined style "file"

clang-format output for Mixer.cpp:

click here for examples.

clang-tidy output for Mixer.cpp:

/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:109:20: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
        if( renderOnly == false )
            ~~~~~~~~~~~~~~^~~~~
            !renderOnly                                                                                                                                                                                                                      
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:306:64: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
        return cpuLoad() >= 99 && Engine::getSong()->isExporting() == false;
                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
                                  !Engine::getSong()->isExporting()                                                                                                                                                                          
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:348:23: warning: invalid case style for static variable 'last_metro_pos' [readability-identifier-naming]
        static Song::PlayPos last_metro_pos = -1;
                             ^~~~~~~~~~~~~~
                             s_last_metro_pos                                                                                                                                                                                                
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:374:18: warning: Potential memory leak [clang-analyzer-cplusplus.NewDeleteLeaks]
                last_metro_pos = p;
                               ^
[...]

/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:405:8: warning: statement should be inside braces [readability-braces-around-statements]
                        else delete *it;
                            ^
                             {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:459:8: warning: statement should be inside braces [readability-braces-around-statements]
                        else delete *it;
                            ^
                             {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:679:25: warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
        if( criticalXRuns() == false )
            ~~~~~~~~~~~~~~~~~~~^~~~~
            !criticalXRuns()
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:690:6: warning: statement should be inside braces [readability-braces-around-statements]
        else delete handle;
            ^
             {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:743:8: warning: statement should be inside braces [readability-braces-around-statements]
                        else delete _ph;
                            ^
                             {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:769:8: warning: statement should be inside braces [readability-braces-around-statements]
                        else delete *it;
                            ^
                             {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:785:25: warning: statement should be inside braces [readability-braces-around-statements]
        if( s_renderingThread )
                               ^
                                {
/home/johannes/cprogs/lmms/master/src/core/Mixer.cpp:807:25: warning: statement should be inside braces [readability-braces-around-statements]
        if( s_renderingThread )
                               ^
                                {
Suppressed 13365 warnings (13365 in non-user code).

Things to discuss:

CMake integration: multiple possibilities:
- No CMake integration
  - Users must use clang-tidy from their IDE, if they use an IDE supporting it,
    otherwise they have to run clang-tidy manually, passing command line args to it
  - No Continous integation!
- Let CMake only export compile commands
  - No full automation
- Always run clang-tidy at compile, if a flag is set
  - Compile time overhead!
  - The need to set that CMake flag may confuse users
- Add a target for CMakeLists to fix (and alternatively just to scan) all source files
  - Disadvantage: Can only accept all fixes, where in an IDE you can select what to fix
Line length: The coding conventions say "80", but it seems to look better using "100".
With 80, many QObject::connect calls are broken into 4 lines.

For the future, I hope we can fix the complete LMMS sources after stable-1.2 is being merged.

JohannesLorenz · 2018-11-01T21:19:14Z

Per 0b5a7d8, function call arguments are now not separated by newlines, so e.g. calls to QObject::connect are more compact now.

JohannesLorenz · 2018-11-01T21:20:54Z

Explanation about the constructor lists: As you can see, now multiple initializers from the CTOR initializer list are put into one line. I find that ugly, but it looks like clang-format does not provide any workaround here.

JohannesLorenz · 2018-11-02T17:07:31Z

A few more examples: JohannesLorenz/lmms@bdde4e3 shows code-reading rework I did automized, and JohannesLorenz/lmms@1e091ea shows rework that remained to do by hand. As one can see, types are corrected automatically, while variables can currently not being checked (due to clang-format which can not differ between s_ and m_ variable prefixes).

.clang-format

JohannesLorenz · 2019-01-01T16:05:17Z

Side note: The tab width (4 or 8) is not clearly defined within the project. We need to find that out.
Edit: solved.

Reflexe · 2019-05-27T19:02:54Z

#4810 states that we should change to spaces (4) instead of tabs. clang-format could save me a lot of time instead of trying to guess every project's code style (and then force-push style changes). Also, clang-tidy could help us to modernize the codebase (e.g. foreach instead of the old "iterator for", auto in casts, and more)

Spekular · 2019-05-27T19:09:46Z

Looks to me like tabs of length four have most votes in each poll, am I missing somewhere where spaces are decided on?

…

On Mon, May 27, 2019, 21:02 Shmuel H. ***@***.***> wrote: #4810 <#4810> states that we should change to spaces (4) instead of tabs. clang-format could save me a lot of time instead of trying to guess every project's code style (and then force-push style changes). Also, clang-tidy could help us to modernize the codebase (e.g. foreach instead of the old "iterator for", auto in casts, and more) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4690?email_source=notifications&email_token=ACEBLGSFUKQAZFCA35VOLFDPXQV6HA5CNFSM4GAZG6C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWKLY5Q#issuecomment-496286838>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEBLGWMEGGHBJZJSXICM6DPXQV6HANCNFSM4GAZG6CQ> .

Reflexe · 2019-05-27T19:13:36Z

Looks to me like tabs of length four have most votes in each poll, am I missing somewhere where spaces are decided on?

Im sorry. I got confused between the emojis.

JohannesLorenz · 2019-05-30T19:30:35Z

Tab width fixed to 4.

Reflexe · 2019-05-31T08:38:38Z

Maybe we should just use Qt's code style with 4-width tabs? It will be a lot easier to set up with qtcreator and clang-format. Also, it takes me so much time and frustration to manually format my code every time.

…

On Thu, 30 May 2019, 22:30 Johannes Lorenz, ***@***.***> wrote: Tab width fixed to 4. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4690?email_source=notifications&email_token=AFOMGKASTPUSF6PXQUCI5WDPYATOJA5CNFSM4GAZG6C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWTJCVA#issuecomment-497455444>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFOMGKAG4LJTE5ZIA5PJVLTPYATOJANCNFSM4GAZG6CQ> .

JohannesLorenz · 2019-06-23T11:04:22Z

Some more additional info:

You can turn lines off that you really do not want to format (e.g. our constructor initializer lists?):

// clang-format off
...
// clang-format on

There's a github clang formatter app, similar to travis (I think @Reflexe found it out)

JohannesLorenz · 2019-06-23T11:11:01Z

Maybe we should just use Qt's code style with 4-width tabs? It will be a lot easier to set up with qtcreator and clang-format. Also, it takes me so much time and frustration to manually format my code every time.

Setting up a coding style once for QtCreator is no big deal. Just create an XML file and you can always re-use this file. Setting it up for clang-format won't be easier: With this PR, we already have a setup for clang-format.

I'm not sure if Qt's code style will match ours. At least for some variable names, this is true, but every derivation will cause more changes in our code base, which is why I was for defining our own code style.

JohannesLorenz · 2019-06-23T11:37:27Z

I find it problematic that this PR is almost ignored. If properly set up, it can increase PR review time a lot, but the points that need to be discussed (as mentioned in the first post) are not discussed at all. The example images/outputs have also not been discussed.

1 or 2 persons can't decide anything that will have significant influence for all active coders. So I'm going to tag everyone to find a consent. Please read the posts carefully, check the examples, make proposals, and if in doubt, try it out on your machine.

Sawuare · 2019-06-23T16:08:06Z

@JohannesLorenz I agree with you on increasing the column limit to 100.

tresf · 2019-06-23T16:18:11Z

column limit to 100 instead of 80?

Why? Because you picked 100? Just get rid of it.

Spekular · 2019-06-23T16:32:28Z

At the very least 120 imo, otherwise the increase is marginal.

…

On Sun, Jun 23, 2019, 18:18 Tres Finocchiaro ***@***.***> wrote: column limit to 100 instead of 80? Why? Because you picked 100? Just get rid of it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4690?email_source=notifications&email_token=ACEBLGWKSFUN74FMKX4NA5DP36O4RA5CNFSM4GAZG6C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYLB6LQ#issuecomment-504766254>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEBLGU5UMNT7FWEBIA34H3P36O4RANCNFSM4GAZG6CQ> .

JohannesLorenz · 2019-06-23T17:48:37Z

Line width is not only about editor for me. E.g. take this line:

m_syncData->m_latency = m_syncData->m_bufferSize * m_syncData->m_bpm / ( (float) m_syncData->m_sampleRate * 60 );

Do you really want to read that? You could make this more readable by using:

<whatever type>& d = m_syncData;
d->m_latency = d->m_bufferSize * d->m_bpm / ( (float) d->m_sampleRate * 60 );

tresf · 2019-06-23T18:25:23Z

... but it's misplaced to use arbitrary values to fix unreadable code. It's an anecdotal fix. Code reviews will still be required for human readibility. A computer can't automate all human tasks.

tresf · 2019-06-23T18:28:28Z

Also, some of the advanced editors can help simplify and clean code using hints. JetBrains has excellent code helpers for this stuff.

JohannesLorenz · 2019-06-23T20:25:06Z

Why? Because you picked 100? Just get rid of it.

I'm not sure if clang-format works sane without any line length limit (or a limit of, say 200). I guess it would concatenate almost every multi-liner into a very very long line... Maybe this must be tested.

Btw, @douglasdgi Sorry, I forgot you in the list of "active coders"

JohannesLorenz · 2021-08-06T19:14:01Z

Open question from the OP:

CMake integration: multiple possibilities:
- No CMake integration
  - Users must use clang-tidy from their IDE, if they use an IDE supporting it,
    otherwise they have to run clang-tidy manually, passing command line args to it
  - No Continous integation!
- Let CMake only export compile commands
  - No full automation
- Always run clang-tidy at compile, if a flag is set
  - Compile time overhead!
  - The need to set that CMake flag may confuse users
- Add a target for CMakeLists to fix (and alternatively just to scan) all source files
  - Disadvantage: Can only accept all fixes, where in an IDE you can select what to fix

What's your opinion?

Veratil · 2021-08-06T20:10:19Z

Automation is usually the easiest route. We'd need to make it clear that it's running so anyone doesn't wonder why their code (possibly) changed from what they originally wrote though.

To be clear I'm saying it should auto-run at some step. Maybe a git hook?

he29-net · 2021-08-06T20:33:46Z

Having a cmake flag available to set clang-tidy to run at compile time would be nice either way I think. People who do not like the overhead or don't want to set the flag can still run it manually (or set it up in their IDE), so the first and third options should not be mutually exclusive.

In CI, I assume it would do scan-only (by utilizing the cmake flag?), and reject the PR if it finds issues. PR authors should read about the coding style before making a contribution, and it would tell them to either set the cmake flag or to run clang-tidy / clang-format manually to easily meet the requirements. The CI job could also remind them ("Did you forget to run clang-*?") to make it clear why it failed.

I can't really comment on the "export-only" variant since I don't quite understand what it means (but it seems like a complication compared to a standard cmake .. && make?). And the last option seems to have not enough control: what if some user updates their clang-* and it insists on fixing some newly detected issue that is not yet fixed in the LMMS master branch? It would force them to include the fix in their PR, even if it is completely unrelated to their changes. So a way to exclude files from the check / formatting could be quite useful..

JohannesLorenz · 2021-08-07T20:11:38Z

Automation is usually the easiest route. We'd need to make it clear that it's running so anyone doesn't wonder why their code (possibly) changed from what they originally wrote though.

FWIR, we agreed (in discord) on not modifying anything in the PR.

Personally, I think it would be OK if the bot creates a branch like "-bot" (and removes that branch after the merge). This allows the user to fetch the bot changes if they want, but it does not modify the PR. What about that?

JohannesLorenz · 2021-08-07T20:22:20Z

Having a cmake flag available to set clang-tidy to run at compile time would be nice either way I think. People who do not like the overhead or don't want to set the flag can still run it manually (or set it up in their IDE), so the first and third options should not be mutually exclusive.

I would probably not like that too much. Changing the cmake config each time seems annoying to me. Maybe I'll compile and wonder why it takes so long, and only after a while realize that I have clang-tidy enabled while compiling...

I can't really comment on the "export-only" variant since I don't quite understand what it means (but it seems like a complication compared to a standard cmake .. && make?).

The idea is that clang-tidy must know how each source file is compiled (which defines and which includes are used). So instead of running it directly from the cmake call, we let cmake just output one large file with compile commands for each source file. Then, we let clang-tidy use that file in a second step.

And the last option seems to have not enough control: what if some user updates their clang-* and it insists on fixing some newly detected issue that is not yet fixed in the LMMS master branch? It would force them to include the fix in their PR, even if it is completely unrelated to their changes. So a way to exclude files from the check / formatting could be quite useful..

Agreed. That's probably why I suggested in the previous post to let the CI do all and just pull it: The CI will not update their clang-format version (as long as we don't want that).

tresf · 2021-08-08T04:45:25Z

Personally, I think it would be OK if the bot creates a branch like "-bot" (and removes that branch after the merge). This allows the user to fetch the bot changes if they want, but it does not modify the PR. What about that?

I'd rather not pollute the branches (we can't create them for forks and this could quickly pollute our own). If possible, scripting code change recommendations is best, then they can just accept or ignore the recommendation, but without any intermediary branches.

JohannesLorenz · 2021-08-08T07:46:22Z

I'd rather not pollute the branches (we can't create them for forks and this could quickly pollute our own). If possible, scripting code change recommendations is best, then they can just accept or ignore the recommendation, but without any intermediary branches.

We could simply create a separate LMMS fork, like "LMMS/bot-suggestions". Then our "LMMS/lmms" fork will not be polluted, and like I mentioned, I'd let the bot remove its branches after the original branches have been merged anyways. So I don't think this would end up in pollution.

he29-net · 2021-08-08T08:41:16Z

Maybe I'll compile and wonder why it takes so long, and only after a while realize that I have clang-tidy enabled while compiling...

How much extra time it takes, roughly? Although I agree that doing repeatedly something that needs to be run only once before the final submission may not be very efficient use of CPU cycles regardless.

The idea is that clang-tidy must know how each source file is compiled (which defines and which includes are used). [...] Then, we let clang-tidy use that file in a second step.

The export-only option could be OK as well in that case, if it just saves data for a subsequent manual clang-tidy run. Either way seems better to me than making and managing a bunch of new branches "in the cloud" for something that could be easily done locally.

The cloud solution could also make git blame less useful: imagine you change three lines in a file made by somebody else and they are all touched by the formatting bot. You pull the changes and now the bot is the author of these three lines. When I see these lines and I want to ask the author about them, I would have to figure out which PR it came from, since the name of the true author was lost. And that again may be hard, since the commit message probably won't contain the PR number, and the bot branch itself may be long deleted.

JohannesLorenz · 2021-08-08T09:43:47Z

Maybe I'll compile and wonder why it takes so long, and only after a while realize that I have clang-tidy enabled while compiling...

How much extra time it takes, roughly? Although I agree that doing repeatedly something that needs to be run only once before the final submission may not be very efficient use of CPU cycles regardless.

Interestingly, now only 9 seconds (on a normal desktop CPU). When I ran it the first times with tons of changes, it took multiple minutes for a whole run (2 or 3 seconds per file). So runtime seems to be no issue anymore (contrary to what I first wrote/thought). So we could integrate it into cmake. -> Wrong, see post below.

The cloud solution could also make git blame less useful.

I think the question is how it looks on master. Either the whole branch is a "feature; fix; fix; fix", in which case you just squash into one commit, which you author (most cases in LMMS). Or, the branch commit history is good enough for not being squashed - in this case, I'd expect the developer to not keep "fixed format" commits on the branch, but squash them into the commits where they really belong (e.g. using git rebase -i and using f in the rebase editor). So on master, I'd expect no bot commits in either case.

tresf · 2021-08-08T14:21:23Z

If a developer changes a line, that's the diff they expect to see. Changing what they changed again sounds like a cat chasing its tail. Code formatting can be yelled at by the CI without adding pandemonium to an otherwise predictable coding workflow.

JohannesLorenz · 2021-08-08T14:51:50Z

Correcting my own statement:

Interestingly, now only 9 seconds (on a normal desktop CPU). When I ran it the first times with tons of changes, it took multiple minutes for a whole run (2 or 3 seconds per file). So runtime seems to be no issue anymore (contrary to what I first wrote/thought). So we could integrate it into cmake.

Those 9 seconds were for clang-format. For clang-tidy, it is still very slow (~ as slow as for a normal compile, so it doubles compiling time). So IMO a flag to activate it would be not so good.

JohannesLorenz · 2021-08-08T14:53:20Z

If a developer changes a line, that's the diff they expect to see. Changing what they changed again sounds like a cat chasing its tail. Code formatting can be yelled at by the CI without adding pandemonium to an otherwise predictable coding workflow.

Sorry, I can't follow what you mean. Of course, if they submit changes that are against our coding conventions, the bot must submit changes to their changes.

tresf · 2021-08-08T15:05:41Z

Sorry, I can't follow what you mean.

Don't touch other's code without their approval (or abandonment). Ever. It's a mistake.

the bot must submit changes to their changes.

The bot MUST do no such thing. Good code is more important than formatted code. The CI can yell at them and they can fix it manually. If you get creative, make code recommendations using GitHub comments. Anything that changes a PR without permission is a mistake. Creating rogue branches is a mistake. Don't make code formatting more important than the code itself (pretty please). ❤️

JohannesLorenz · 2021-08-08T16:30:18Z

You mean the CI gives the author hints, and they can fix it, but are not forced to take it? Do you propose that the reviewer would still check that the relevant bot hints were used (which means manual review again, which we wanted to avoid?). Or should the reviewer then just say "Author, I trust you did the formatting well"?

Another problem with this is: If you do changes A, B and C, deny clang-format's changes and bring it on master. Then the next coder who wants to do change D will be confronted with issues from A, B, C and D. In the end, the whole file will be marked with errors from clang-*, and we get more and more away from clang-*'s suggestions. Of course, you can still manually pass the line numbers to clang-* (this is another parameter you may specify), but if even your changed lines are already full of previous errors... Also, specifying the line numbers you changed can get difficult if you changed lines in 10 places of a file - there are already scripts that feed clang-* with the git diff, but do we want to use scripts for that? (such script should work on Windows, too...)

Summing it up, I'm for either use clang-* and always keep all files 100% consistent, or to not use clang-* at all. But what do the other coders say?

he29-net · 2021-08-08T18:21:05Z

From my point of view, there are two key "big picture" requirements that out clang-* deployment should meet:

Make it mandatory.
Apart from "making code look nice", eliminating manual code style checking from the review process is one of the main goals. If there are any clang-* issues left in the LMMS code base, the CI job would be pretty useless, since it would just fail every time (or someone would have to keep managing the exceptions).
That means it would still require attention from reviewers, and a quick look on the PR tracker shows that their attention would be better focused elsewhere. So I definitely support the "no exceptions"* idea; either the PR meets the formatting requirements or it does not get merged. I'm sure that having good and formatted code at the same time isn't exclusive.
* perhaps excluding plugins or 3rd-party folders, since forcing our style on these may conflict with upstream
Make it easy to use.
In order to make it a mandatory requirement for PR acceptance, it must be also easy to use. Shifting some of the work from reviewers to CI and PR authors is good, but some people have already trouble compiling LMMS for the first time (mainly on Windows), so adding another obstacle for the newcomers would not be helpful. This should also include multiple ways to achieve conformity: simple manual solutions for casual devs who are submitting just a small 5 line patch for the first time, and advanced options for devs that want more automation and are willing to invest some time up front to set things up.

Going deeper into 2): For me, the "cloud bot approach" (create a branch with suggested changes that the PR author can use) seems complicated and messy, but for some devs it may be preferred. Others like me could prefer an option that lets me easily run clang-* locally. So why not support both, so that everyone can find something that works best for them?

Both "cloud" and "local" solutions may be still kind of difficult for some people, though: one requires relatively advanced git knowledge (I did not even know the "interactive rebase" existed before Johannes told me last year), the other requires extra tools that Windows users may need to set up.
So as a fallback, it should be also easy to fix problems manually by editing the code. That's why I think a simple CI job that says "hey, your code failed the formatting check, here is a log that says what's wrong" may be more useful in this case than a new branch appearing somewhere in a different repo, where the user has to locate the appropriate commit, which then probably contains only the suggested fixes, but no explanation of what was wrong. (But again, the CI output could contain the log and a link to the branch with suggested fixes, these are also not exclusive..)

tresf · 2021-08-09T14:37:37Z

adding another obstacle for the newcomers would not be helpful.

This is not true. Automating the "sorry your code doesn't pass our style guidelines" DOES help the user. Otherwise they're waiting for days, weeks or months for a human to do it. This notion that early users can't fix formatting or that it's a detriment to notify them by bot is ludicrous. Our job is NOT to write other people's code and the more we conflate the workflow and take the responsibility off of the coder, the less we expect of our coders. I'll keep using Homebrew as an example here because they merge hundreds and hundreds of PRs every week and they do NOT make changes for people. Homebrew is a Ruby project, so we're talking about a lower barrier to entry. I don't think we should do anything automatically that touches other people's code unless they explicitly request it. Once code is compiling well, I think developers will be happy to fix formatting errors to get it merged.

Going deeper into 2): For me, the "cloud bot approach" (create a branch with suggested changes that the PR author can use) seems complicated and messy, but for some devs it may be preferred. Others like me could prefer an option that lets me easily run clang-* locally. So why not support both, so that everyone can find something that works best for them?

Preferred by whom? I've contributed to several dozen projects, none do code formatting forking nonsense. I really don't understand any use-case where creating forks for formatting is warranted. Forks should be used for code divergence, not for making sure there's a newline and no spaces after a {.

the CI output could contain the log and a link to the branch with suggested fixes

Homebrew's errors just cite the rule and I believe sometimes links to examples.

You mean the CI gives the author hints, and they can fix it, but are not forced to take it?

Yes.

Do you propose that the reviewer would still check that the relevant bot hints were used

If your workflow requires a passing codestyle, you block merging until it passes. There should always be a manual override though.

the whole file will be marked with errors from clang-, and we get more and more away from clang-'s suggestions.

Ideally, we'd only run checks on stuff that's changed. We need not make the style check more important than the code itself, no?

but if even your changed lines are already full of previous errors... Also, specifying the line numbers you changed can get difficult if you changed lines in 10 places of a file - there are already scripts that feed clang-* with the git diff, but do we want to use scripts for that? (such script should work on Windows, too...)

That's the same for any code that's changed and was previously under review. If a developer chooses to ignore the formatting errors and change lines again, the old code comments should naturally become outdated and collapse by default.

tresf · 2021-08-09T14:45:50Z

it must be also easy to use

I don't think there's anything "easy" about auto-formatting code unless the IDE's doing it as you type. Any solution that happens after the code is written (in my opinion) should require human intervention. This doesn't mean the local scripts wont help, they absolutely will (and the CI can offer some guidance here). This becomes increasingly helpful for PRs that becomes stale and only formatting it preventing it from being merged. Any developer can run the format on the code locally and bump the PR with the formatting changes.

(such script should work on Windows, too...)

For command-line stuff, Windows may be the oddball out here, but from what I'm reading the tidy tasks are supported, at least in the IDEs and will pick up the tidy files automatically.

he29-net · 2021-08-09T15:13:24Z

adding another obstacle for the newcomers would not be helpful.

This is not true. Automating the "sorry your code doesn't pass our style guidelines" DOES help the user. [...]

I am not saying otherwise. As I explained in the post, I agree that having a CI job that rejects non-compliant code is useful, since it saves time of our reviewers. That CI job is not a new obstacle, it just changes who deals with it.

By "adding obstacles" I mean making clang-* tools required for compilation (which could be annoying to Windows users) or making the suggestions only available as a diff in some extra bot-generated fork (which could be annoying to anyone who is not a git expert). If I want to submit a short patch that takes 15 minutes to write and test (my first LMMS PR was literally adding a missing ) to a formula), I don't want to spend two more hours setting up tools that I never used before or learning git dark magic. I just want the CI job to tell me what failed and how can I fix it manually.

[fork based approach]

Preferred by whom?

I don't know, I can't speak for others. I do not like it either, but Johannes seemed to prefer it (unless he was just further explaining what the process could look like and I mistakenly understood it as a preference).

Any solution that happens after the code is written (in my opinion) should require human intervention.

And I agree (and the human should be the PR author, not the reviewer). By "being easy to use" I mean not forcing the user into a specific way to fix the code (which could require complex setup or advanced knowledge), but simply telling them in the CI log what is wrong and how it can be fixed manually, so that literally anyone could do it, even if they just barely managed to even compile LMMS for the first time and fixed a simple bug. I think we actually agree on that point (and most others) and just describe it differently.

JohannesLorenz · 2021-08-09T19:05:38Z

Preferred by whom?

Me

nonsense

Can we talk less destructive?

IDEs will help, and IMO we should do checks prior to merging (I think we agreed until here). And if everyone can easily apply clang-* on their changes, with an identical result on all machines worldwide, all will be great. However, I fear this will not work:

Users will have different versions of the clang-* tools
clang-* will behave different on different systems (due to bugs, CRLF, whatever)
Some users will be too unskilled to handle these tools (especially, not every Windows user may easily install the clang-tools, open a cmd and call the clang-* tools correctly)

All in all, I still expect regular situations where users tried to submit code and the CI rejects them. I expect this not just 1%, but rather something like 10% or 20%. I think if it can happen regularly, we should be prepared for it. So I, personally, don't want to look at a 100 lines diff, fixing each single indentation issue with my own hands. So you may say that the CI could just put the resulting files zipped somewhere. Now compare: Hitting the download button in your browser for the zip, then unzipping it, carefully copying over all the files into you own git tree, typing a git commit, possibly removing the zip and the folder. Compare this with a simple git fetch && git merge bot-fixes/mybranchname (something like this). I think my solution with the bot fixes will be much faster and safer.

Am I too pessimistic about all clang-* worldwide will produce the same output and that problems will occur only in exceptional cases?

he29-net · 2021-08-09T20:10:35Z

Am I too pessimistic about all clang-* worldwide will produce the same output and that problems will occur only in exceptional cases?

I think it is a valid concern, but we would likely not be the only ones hit with such issues, so I doubt that there will be many breaking changes in behavior across versions and platforms that couldn't be fixed by a new config option or something. Big organizations using these tools would probably not be happy if each new version reformatted their whole code-base with not workaround available.

As I noted on Discord, we started using clang-format recently at work, and since day zero we had to add an extra option to the config file, which must be commented out on version 7 (since that's the version we mainly use and it does not know the option), and uncommented on version 11-ish and newer, to keep the formatting consistent with the older clang-format version. So it does happen, but we could likely just provide an alternative config file for new versions that behave differently.

I don't see any harm in having a bot-fork based option for those who want to use it (currently 33 % of participants in this discussion :)) ), but I would definitely like to see a fully supported option to run clang-* locally as well (which is probably also a prerequisite for all the IDE helper things).

tresf · 2021-08-09T20:20:09Z

CRLF, whatever

CRLF is handled by Git. We can turn that off project-side using .gitattributes.

Can we talk less destructive?

Rash perhaps, but destructive? I don't understand what's so confusing about having a CI tell someone to fix their code. These talks of automating this with branches is -- in my opinion -- destructive in a literal sense because it will pollutes and conflate. Downstream, we lack sufficient access to fork, upstream we'll be making forks for formatting? I find it a bit hypocritical to think that formatting code is too complex for someone to grasp while merging in an upstream fork is not complex. One is a matter of following rules, the other requires two remotes and hand-merging or pull requests against forks, neither of which I find to be intuitive to a newcomer. Sorry if that comes off as "destructive", I just find the workflow to be all around messy.

I would definitely like to see a fully supported option to run clang-* locally

I think this is more pragmatically good.

I don't see any harm in having a bot-fork based option for those who want to use it

OpenJDK (especially the CPP-side) does some pretty creative bot stuff for merging PRs, but it's mostly for project integration, I guess for stale branches this might help, I just think it'll cause more PRs in the end. If we have access to push to a stale PR, that's likely the cleaner solution, but I won't fight the bot conversation any longer, I've shared my opinions on it. 🍻

we started using clang-format recently at work, and since day zero we had to add an extra option to the config file, which must be commented out on version 7 (since that's the version we mainly use and it does not know the option), and uncommented on version 11-ish and newer, to keep the formatting consistent with the older clang-format version. So it does happen, but we could likely just provide an alternative config file for new versions that behave differently.

Thanks for sharing. These are the types of exception scenarios I was trying to discuss on Discord.

JohannesLorenz · 2021-08-11T18:04:13Z

I would definitely like to see a fully supported option to run clang-* locally

I think this is more pragmatically good.

For local support, going through the above options:

1. No CMake integration
2. Let CMake only export compile commands
3. Always run clang-tidy at compile, if a flag is set
4. Add a target for CMakeLists to fix (and alternatively just to scan) all source files

So I think (1) is not possible, because we will want to do checks at CI side to forbid ill-formed code. (3) and (4) seem to make it impossible to run CMake only on the changed files. However, imagine changing only a header which is included by 100 cpp files. Then there's no way around re-compiling those 100 cpp files with clang-titdy, too (because the definitions may have changed, resulting in different clang-tidy-fixes)? So is this really a problem? (2) seems nice because you can restrict clang-tidy on the files you really changed, but isn't this exactly the danger with the header that requires re-compiling 100 files?

All in all, I think we can even offer 2, 3 and 4 in parallel. We'll probably see what is best in practice and the CI will save us or errors anyways...

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

Rossmaxx · 2022-05-03T06:38:30Z

isn't this done, why not close it?

JohannesLorenz · 2022-05-04T18:09:26Z

isn't this done, why not close it?

Correct.

Deprecated by #6323 (and merged there).

JohannesLorenz added the enhancement label Nov 1, 2018

JohannesLorenz added this to the 1.3.0 milestone Nov 1, 2018

PhysSong reviewed Nov 8, 2018

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

PhysSong added the in progress label Dec 10, 2018

JohannesLorenz mentioned this pull request Feb 3, 2019

Indentation (tabs or spaces) #4810

Closed

tresf mentioned this pull request Mar 14, 2019

Coding conventions #1826

Closed

This was referenced Apr 7, 2019

Setting up QtCreator (Guide + Scripts) #4934

Open

Paying more attention to PRs #4935

Open

JohannesLorenz added the pr-wip label Apr 17, 2019

PhysSong removed the pr-wip label Apr 22, 2019

JohannesLorenz force-pushed the coding-conventions branch from 0b5a7d8 to 94b48d5 Compare May 30, 2019 19:29

JohannesLorenz mentioned this pull request Jul 24, 2021

Clang tools testing #5948

Closed

PhysSong added the bot-skip tell lmmsbot to ignore this pr label Feb 3, 2022

JohannesLorenz added a commit to JohannesLorenz/lmms that referenced this pull request Mar 5, 2022

Run clang-format on all files (LMMS#4690)

6a2d6fb

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

JohannesLorenz closed this May 4, 2022

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

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

Conversation

JohannesLorenz commented Nov 1, 2018 • edited Loading

JohannesLorenz commented Nov 1, 2018

JohannesLorenz commented Nov 1, 2018

JohannesLorenz commented Nov 2, 2018

JohannesLorenz commented Jan 1, 2019 • edited Loading

Reflexe commented May 27, 2019

Spekular commented May 27, 2019 via email

Reflexe commented May 27, 2019

JohannesLorenz commented May 30, 2019

Reflexe commented May 31, 2019 via email

JohannesLorenz commented Jun 23, 2019

JohannesLorenz commented Jun 23, 2019

JohannesLorenz commented Jun 23, 2019

Sawuare commented Jun 23, 2019

tresf commented Jun 23, 2019 • edited Loading

Spekular commented Jun 23, 2019 via email

JohannesLorenz commented Jun 23, 2019

tresf commented Jun 23, 2019 • edited Loading

tresf commented Jun 23, 2019

JohannesLorenz commented Jun 23, 2019

JohannesLorenz commented Aug 6, 2021

Veratil commented Aug 6, 2021 • edited Loading

he29-net commented Aug 6, 2021

JohannesLorenz commented Aug 7, 2021

JohannesLorenz commented Aug 7, 2021

tresf commented Aug 8, 2021

JohannesLorenz commented Aug 8, 2021

he29-net commented Aug 8, 2021

JohannesLorenz commented Aug 8, 2021 • edited Loading

tresf commented Aug 8, 2021

JohannesLorenz commented Aug 8, 2021

JohannesLorenz commented Aug 8, 2021

tresf commented Aug 8, 2021 • edited Loading

JohannesLorenz commented Aug 8, 2021

he29-net commented Aug 8, 2021

tresf commented Aug 9, 2021

tresf commented Aug 9, 2021

he29-net commented Aug 9, 2021

JohannesLorenz commented Aug 9, 2021

he29-net commented Aug 9, 2021

tresf commented Aug 9, 2021 • edited Loading

JohannesLorenz commented Aug 11, 2021

Rossmaxx commented May 3, 2022

JohannesLorenz commented May 4, 2022

JohannesLorenz commented Nov 1, 2018 •

edited

Loading

JohannesLorenz commented Jan 1, 2019 •

edited

Loading

tresf commented Jun 23, 2019 •

edited

Loading

tresf commented Jun 23, 2019 •

edited

Loading

Veratil commented Aug 6, 2021 •

edited

Loading

JohannesLorenz commented Aug 8, 2021 •

edited

Loading

tresf commented Aug 8, 2021 •

edited

Loading

tresf commented Aug 9, 2021 •

edited

Loading