Apply clang-format for unified coding-style

[hjtappe]

trafficstars

Today, the coding style check is a manual review process, so I wondered if this could not be done by a static code analysis tool to offload that work from @mcallegari as our maintainer.

I first tried how far we could get with the well-established indent program, but style recommendations are too far from the current code and configuration options are not sufficient to get closer.

So I shifted to clang-format (version 14 as in Ubuntu 22.04) and came quite close. Nevertheless, the build system would have version 10 which does not support required options and clang-format is currently moving fast, so I fixed that to version 14. Note that therefore the additional myci-actions/add-deb-repo@*, entry needs to be allowed in the actions configuration.

I applied two configurations, one for *.cpp and *.h and another one for *.js.

Let me know what you think.

• Is it worth it or in the way when developing code?
• Is it the right approach to put it into unittest.sh (rather than as a Make target)?
• Is it close enough to the current formatting?
• Can we live with how it wants the JS files to be formatted?

[mcallegari]

Hi, I'm not sure about this. Some changes are good some other I don't like, especially QString &var vs QString& var. See https://wiki.qt.io/Qt_Coding_Style Except for curly braces (which for Christ sake they go on newlines!) I almost agree with that as it is. Can you please re-run the parsing with adjusted settings? Thanks

[yestalgia]

Anything that makes the code easier to maintain is definitely a plus. @mcallegari would you approve if we used their clang profile?

https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format

[hjtappe]

The QLC style to be found in the current files differs significantly from the rules to be found at https://code.qt.io/cgit/qt/qt5.git/tree/_clang-format.

I'll modify the QLCplus rule set to get as close to the existing code with taking the original QT rules in mind and upload a re-ordered version of the original QT rules here, so they can be compared to the QLCplus configuration. It can then be decided what we want to see (with my current assumption: as few as possible changes to the existing and established code, which will then be consisten).

[hjtappe]

So now the pointers are on right-aligned. This creates lines like: return qobject_cast<AudioDecoder *>(copy);

The attached file clang-format-code.qt.io.txt can now be diffed to the QLCplus .clang-format file. I have added the comments from the QT file for better comparability, but in version 14, there are more options.

Please note that the check is currently not applied to the following cpp directories because they do not strictly follow the rules in their original fashion:

• plugins
• fixtureeditor
• hotplugmonitor

For a batch update , the unittest.sh can be modified with the -i inplace update and the removal of the error exit if anyone wants to play with the configuration options: `--- a/unittest.sh +++ b/unittest.sh @@ -65,12 +65,12 @@ find
webaccess
-name '.h' -and -not -name 'moc_' -and -not -name 'ui_' -and -not -name 'qlcconfig.h' -or
-name '.cpp' -and -not -name 'moc_' -and -not -name 'qrc_' | while read FILE; do

• clang-format-14 -style=file:.clang-format "$FILE" | diff $DIFFARG "$FILE" -

• clang-format-14 -i -style=file:.clang-format "$FILE" | diff $DIFFARG "$FILE" - RET=$? if [ $RET -ne 0 ]; then echo >&2 "$FILE: Error in formatting. Run:" echo >&2 " clang-format-14 -i -style=file:.clang-format "$FILE""

• exit $RET +# exit $RET else echo >&2 "$FILE: Formatting OK" fi`

[mcallegari]

@hjtappe sorry but I've got some more things that I would adjust:

• switch/case should look like this (and yes, I don't like Qt style in this case)

switch (var)
{
    case 1:
    {
        // parenthesis needed only if a variable is defined in this block
    }
    break;
    default:
    break;
}

• defines/includes should start from the beginning of the line unless nested to other defines
• please leave aligned enum/define values, they're easier to read. Example: https://github.com/mcallegari/qlcplus/blob/master/engine/audio/src/audiocapture.h#L34 https://github.com/mcallegari/qlcplus/blob/master/engine/src/function.h#L110
• I saw multiple rows packed into a single line. Here there should be a limit to text columns. I think 80 columns is a reasonable value

[hjtappe]

Hello,

@hjtappe sorry but I've got some more things that I would adjust:

No problem. You can even still decide it that is the way to go or to cancel this and go another way. ;-) Otherwise, please allow the additional myci-actions/add-deb-repo@*, entry in the github action, so github actions will be willing to build the code.

* switch/case should look like this (and yes, I don't like Qt style in this case)

I think that's now the case.

* defines/includes should start from the beginning of the line unless nested to other defines

Can you point me to a file where you see a misbehaviour, please? Looking at e.g. engine/src/doc.cpp, it indents according to the preprocessor expression level. Do you refer to that it should always start in the first column?

* please leave aligned enum/define values, they're easier to read. Example:
  https://github.com/mcallegari/qlcplus/blob/master/engine/audio/src/audiocapture.h#L34
  https://github.com/mcallegari/qlcplus/blob/master/engine/src/function.h#L110

I think I found the switch.

* I saw multiple rows packed into a single line. Here there should be a limit to text columns. I think 80 columns is a reasonable value

80 will have a significant breaking impact on code. Other standard configurations use 100 rather than 120. I have set that - please take another look if that could also fit our needs.

[mcallegari]

Hey @hjtappe sorry for leaving this behind! I was thinking: is it possible to split this into several PRs or separate commits? First I would tackle QString& var vs QString &var (same for *) and if( vs if ( (same for while, for, foreach, switch, etc) I think that would cover already many cases in the existing code. Then we can discuss other specific styles and the application of an automated style check.

Might help me to review this. Thanks

[hjtappe]

Hello @mcallegari, technically this does not work with the indentation program. It can't be told to apply only one set of rules, as far as I see. I could nevertheless accomplish that with small "sed" scriptlets and run that on the current master before a careful review. I'll give that a try.

I wanted to proceeed with this and some other topics in the previous month, but life became too busy the last months. I think I can address this again next year and will set it to draft until I have made the next step.

[hjtappe]

Hello @mcallegari,

after some research - it is not possible to adress only the pointers indentation only. For a script solution, the pointer "wildcard" character is used in too many places and need semantics understanding of the character environments.

The tools helping in indentation all cannot be configured to fix only the pointers.

So let's approach the situation from another side.

So I provide the project with a script to help checking single files or directories and also to use for the indentation itself. This way, you (and whoever works to unify the code) can selectively update the code that is currently worked on.

-c|--check Script mode: check (default) -h|--help Print this help message -i|--indent Script mode: indent -t|--target target Build Target (qmlui|ui) default: qmlui -u|--unify Add unified diff output

Later integration into unittest.sh and the build workflows is prepared but requires the clang-format is added to the commandline via Github actions permissions through myci-actions/add-deb-repo@11.

Does that work for you?

[mcallegari]

How about this instead of inventing our own script? https://github.com/google/styleguide/tree/gh-pages/cpplint I haven't tried it yet but it looks you can pass specific filters from command line