Jamulus.pro: Implement (n)make clang_format

[hoffie]

Short description of changes

This simplifies submission of properly formatted code for users with no editor/IDE integration of clang-format (#1702).
It had previously been submitted as #1741 with larger scope (auto-fixing on build). This part has been removed.

Context: Fixes an issue?

Pays in on #1702.

Does this change need documentation? What needs to be documented and how?

• Contribution docs?

Status of this Pull Request

Ready to be tested/reviewed.

What is missing until this pull request can be merged?

• Tests (make clang_format):
  • Linux
  • Windows
  • Mac
• Review

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want (on Linux; other platforms as mentioned above)
• My code follows the style guide no relevant style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[pljones]

Should we include MacOS and iOS *.mm files?

[hoffie]

In general, probably yes, but I'd rather do this in (or after) the existing PR #1871 after all of the actual details have been figured out.

[pljones]

Aha!

[henkdegroot]

@hoffie, I downloaded this branch and tested on my windows machine using the command: nmake clang_format
This is working and updates the files which have incorrect format. I did not want to find/install make for windows, hence why I used the nmake command which is installed as part of the visual studio.

[hoffie]

I did not want to find/install make for windows, hence why I used the nmake command which is installed as part of the visual studio.

Thanks, even better. I think this is the more relevant test case for Windows, but I didn't think of it. Thanks!

[softins]

I've just tried out this PR on my Ubuntu 20 VM, which I have used for various builds, including Android and the .deb. When generating the Makefile, Jamulus.pro finds a lot of unnecessary files. This is the clang-format command that was generated in the Makefile, with all the arguments sorted and split into separate groups for ease of reading:

clang-format -i
        android-build/androiddebug.cpp android-build/sound.cpp android-build/sound.h

        android/androiddebug.cpp android/sound.cpp android/sound.h

        arm64-v8a/moc_analyzerconsole.cpp arm64-v8a/moc_audiomixerboard.cpp arm64-v8a/moc_channel.cpp
        arm64-v8a/moc_chatdlg.cpp arm64-v8a/moc_client.cpp arm64-v8a/moc_clientdlg.cpp
        arm64-v8a/moc_clientsettingsdlg.cpp arm64-v8a/moc_connectdlg.cpp arm64-v8a/moc_creaperproject.cpp
        arm64-v8a/moc_jamcontroller.cpp arm64-v8a/moc_jamrecorder.cpp arm64-v8a/moc_levelmeter.cpp
        arm64-v8a/moc_multicolorled.cpp arm64-v8a/moc_predefs.h arm64-v8a/moc_protocol.cpp
        arm64-v8a/moc_server.cpp arm64-v8a/moc_serverdlg.cpp arm64-v8a/moc_serverlist.cpp
        arm64-v8a/moc_settings.cpp arm64-v8a/moc_signalhandler.cpp arm64-v8a/moc_socket.cpp
        arm64-v8a/moc_sound.cpp arm64-v8a/moc_soundbase.cpp arm64-v8a/moc_testbench.cpp
        arm64-v8a/moc_util.cpp

        armeabi-v7a/moc_analyzerconsole.cpp armeabi-v7a/moc_audiomixerboard.cpp armeabi-v7a/moc_channel.cpp
        armeabi-v7a/moc_chatdlg.cpp armeabi-v7a/moc_client.cpp armeabi-v7a/moc_clientdlg.cpp
        armeabi-v7a/moc_clientsettingsdlg.cpp armeabi-v7a/moc_connectdlg.cpp armeabi-v7a/moc_creaperproject.cpp
        armeabi-v7a/moc_jamcontroller.cpp armeabi-v7a/moc_jamrecorder.cpp armeabi-v7a/moc_levelmeter.cpp
        armeabi-v7a/moc_multicolorled.cpp armeabi-v7a/moc_predefs.h armeabi-v7a/moc_protocol.cpp
        armeabi-v7a/moc_server.cpp armeabi-v7a/moc_serverdlg.cpp armeabi-v7a/moc_serverlist.cpp
        armeabi-v7a/moc_settings.cpp armeabi-v7a/moc_signalhandler.cpp armeabi-v7a/moc_socket.cpp
        armeabi-v7a/moc_sound.cpp armeabi-v7a/moc_soundbase.cpp armeabi-v7a/moc_testbench.cpp
        armeabi-v7a/moc_util.cpp

        build-gui/moc_analyzerconsole.cpp build-gui/moc_audiomixerboard.cpp build-gui/moc_channel.cpp
        build-gui/moc_chatdlg.cpp build-gui/moc_client.cpp build-gui/moc_clientdlg.cpp
        build-gui/moc_clientsettingsdlg.cpp build-gui/moc_connectdlg.cpp build-gui/moc_creaperproject.cpp
        build-gui/moc_jamcontroller.cpp build-gui/moc_jamrecorder.cpp build-gui/moc_levelmeter.cpp
        build-gui/moc_multicolorled.cpp build-gui/moc_predefs.h build-gui/moc_protocol.cpp
        build-gui/moc_server.cpp build-gui/moc_serverdlg.cpp build-gui/moc_serverlist.cpp
        build-gui/moc_settings.cpp build-gui/moc_signalhandler.cpp build-gui/moc_socket.cpp
        build-gui/moc_sound.cpp build-gui/moc_soundbase.cpp build-gui/moc_testbench.cpp
        build-gui/moc_util.cpp
        build-gui/src/res/qrc_resources.cpp
        build-gui/ui_aboutdlgbase.h build-gui/ui_chatdlgbase.h build-gui/ui_clientdlgbase.h
        build-gui/ui_clientsettingsdlgbase.h build-gui/ui_connectdlgbase.h build-gui/ui_serverdlgbase.h

        build-nox/moc_channel.cpp build-nox/moc_client.cpp build-nox/moc_creaperproject.cpp
        build-nox/moc_jamcontroller.cpp build-nox/moc_jamrecorder.cpp build-nox/moc_predefs.h
        build-nox/moc_protocol.cpp build-nox/moc_server.cpp build-nox/moc_serverlist.cpp
        build-nox/moc_settings.cpp build-nox/moc_signalhandler.cpp build-nox/moc_socket.cpp
        build-nox/moc_sound.cpp build-nox/moc_soundbase.cpp build-nox/moc_testbench.cpp
        build-nox/moc_util.cpp
        build-nox/src/res/qrc_resources.cpp

        ios/ios_app_delegate.h ios/sound.h

        linux/sound.cpp linux/sound.h

        mac/activity.h mac/sound.cpp mac/sound.h

        src/analyzerconsole.cpp src/analyzerconsole.h src/audiomixerboard.cpp
        src/audiomixerboard.h src/buffer.cpp src/buffer.h
        src/channel.cpp src/channel.h src/chatdlg.cpp
        src/chatdlg.h src/client.cpp src/client.h
        src/clientdlg.cpp src/clientdlg.h src/clientsettingsdlg.cpp
        src/clientsettingsdlg.h src/connectdlg.cpp src/connectdlg.h
        src/global.h src/levelmeter.cpp src/levelmeter.h
        src/main.cpp src/multicolorled.cpp src/multicolorled.h
        src/protocol.cpp src/protocol.h src/recorder/creaperproject.cpp
        src/recorder/creaperproject.h src/recorder/cwavestream.cpp src/recorder/cwavestream.h
        src/recorder/jamcontroller.cpp src/recorder/jamcontroller.h src/recorder/jamrecorder.cpp
        src/recorder/jamrecorder.h src/server.cpp src/server.h
        src/serverdlg.cpp src/serverdlg.h src/serverlist.cpp
        src/serverlist.h src/serverlogging.cpp src/serverlogging.h
        src/settings.cpp src/settings.h src/signalhandler.cpp
        src/signalhandler.h src/socket.cpp src/socket.h
        src/soundbase.cpp src/soundbase.h src/testbench.h
        src/threadpool.h src/util.cpp src/util.h
        src/vstmain.cpp src/vstmain.h src/vstsound.h

        windows/sound.cpp windows/sound.h

        x86/moc_analyzerconsole.cpp x86/moc_audiomixerboard.cpp x86/moc_channel.cpp
        x86/moc_chatdlg.cpp x86/moc_client.cpp x86/moc_clientdlg.cpp
        x86/moc_clientsettingsdlg.cpp x86/moc_connectdlg.cpp x86/moc_creaperproject.cpp
        x86/moc_jamcontroller.cpp x86/moc_jamrecorder.cpp x86/moc_levelmeter.cpp
        x86/moc_multicolorled.cpp x86/moc_predefs.h x86/moc_protocol.cpp
        x86/moc_server.cpp x86/moc_serverdlg.cpp x86/moc_serverlist.cpp
        x86/moc_settings.cpp x86/moc_signalhandler.cpp x86/moc_socket.cpp
        x86/moc_sound.cpp x86/moc_soundbase.cpp x86/moc_testbench.cpp
        x86/moc_util.cpp

        x86_64/moc_analyzerconsole.cpp x86_64/moc_audiomixerboard.cpp x86_64/moc_channel.cpp
        x86_64/moc_chatdlg.cpp x86_64/moc_client.cpp x86_64/moc_clientdlg.cpp
        x86_64/moc_clientsettingsdlg.cpp x86_64/moc_connectdlg.cpp x86_64/moc_creaperproject.cpp
        x86_64/moc_jamcontroller.cpp x86_64/moc_jamrecorder.cpp x86_64/moc_levelmeter.cpp
        x86_64/moc_multicolorled.cpp x86_64/moc_predefs.h x86_64/moc_protocol.cpp
        x86_64/moc_server.cpp x86_64/moc_serverdlg.cpp x86_64/moc_serverlist.cpp
        x86_64/moc_settings.cpp x86_64/moc_signalhandler.cpp x86_64/moc_socket.cpp
        x86_64/moc_sound.cpp x86_64/moc_soundbase.cpp x86_64/moc_testbench.cpp
        x86_64/moc_util.cpp

So I think either it needs to exclude android-build, arm64-v8a, armeabi-v7a, x86 and x86_64 (used for building Android), and build-gui and build-nox (used for building Debian). Or alternatively, and preferably, only look in android, ios, linux, mac, src and windows.

My run died on build-gui/src/res/qrc_resources.cpp, but that was the VM running out of memory.

[hoffie]

So I think either it needs to exclude android-build, arm64-v8a, armeabi-v7a, x86 and x86_64 (used for building Android), and build-gui and build-nox (used for building Debian). Or alternatively, and preferably, only look in android, ios, linux, mac, src and windows.

Thanks. I didn't think about android at all. :(

I really wanted to avoid whitelisting specific directories, but I no longer think that blacklisting is feasible either. I've therefore updated the code to scan for all .cpp/.h files, filter them by the directory list you gave and remove some special cases in addition (nsProcess, qrc_resources.cpp).

[ann0see]

Marking Windows as tested by henk.

[ann0see]

Tested in macOS VM and it worked. The commit should say (n)make clang_format not (n)make clang-format since the command to run it is not make clang-format

[hoffie]

Tested in macOS VM and it worked. The commit should say (n)make clang_format not (n)make clang-format since the command to run it is not make clang-format

Thanks for testing and the hint. Commit description updated.

[ann0see]

I think the last missing thing is adding it to the CONTRIBUTING.md file.

Maybe we could also modify the PR template to hint at running (n)make clang_format right next to the checkbox with the style guide - or we could remove the style guide checkbox all together since the CI already checks it for us.

[ann0see]

Needs a rebase (and should be merged…)

[hoffie]

Rebased.

I think the last missing thing is adding it to the CONTRIBUTING.md file.

Added.

Maybe we could also modify the PR template to hint at running (n)make clang_format right next to the checkbox with the style guide - or we could remove the style guide checkbox all together since the CI already checks it for us.

Yes, we can do that, but I'd rather get done with this PR and I'd say that it'd be better done in a separate one.

[pljones]

I'm assuming the regex syntax is right or it wouldn't work. Right? (| rather than \| is what made me wonder.)

[hoffie]

I'm assuming the regex syntax is right or it wouldn't work. Right? (| rather than \| is what made me wonder.)

I have confirmed that the excludes work, but you are right, this is suspicious. I'll check that again and mark it as Draft again.

[softins]

I'm assuming the regex syntax is right or it wouldn't work. Right? (| rather than \| is what made me wonder.)

I have confirmed that the excludes work, but you are right, this is suspicious. I'll check that again and mark it as Draft again.

Looks fine to me. The clang_format target in the generated Makefile includes all the source files that it should, and none of the ones it shouldn't. I tested it by changing an indent setting in .clang-format and then doing:

make clang_format
# show what git says changed
git status
# show what source files were unchanged
find . -type f \( -name \*.cpp -o -name \*.h -o -name \*.mm \) ! -newer .clang-format -ls

[ann0see]

@hoffie is this ready? For me everything seemed to work fine.

[hoffie]

I'm assuming the regex syntax is right or it wouldn't work. Right? (| rather than \| is what made me wonder.)

I have confirmed that the excludes work, but you are right, this is suspicious. I'll check that again and mark it as Draft again.

I've done some further tests. \| seems to match a literal pipe char, so it doesn't work. What does work though is removing the escaping from the brackets. I still think we should keep them for the following reason:

$ cat test.pro 
message(unquoted and unescaped: ())
message(unquoted and escaped: \(\))
message("quoted and unescaped: \(\)")
message("quoted and escaped: \(\)")
message(unquoted and unescaped (no start): ))
message(unquoted and escaped (no start): \))
message("quoted and unescaped (no start): )")
message("quoted and escaped (no start): \)")

$ qmake test.pro 
Project MESSAGE: unquoted and unescaped: ()
Project MESSAGE: unquoted and escaped: ()
Project MESSAGE: quoted and unescaped: ()
Project MESSAGE: quoted and escaped: ()
Project MESSAGE: unquoted and unescaped (no start):       <- that case makes visible what happens
Project MESSAGE: unquoted and escaped (no start): )
Project MESSAGE: quoted and unescaped (no start): )
Project MESSAGE: quoted and escaped (no start): )

The qmake parser seems to be rather tolerant in permitting qmake-relevant characters such as brackets in function parameters and unquoted strings. This only works as long as there are no ambiguities though. We shouldn't rely on that. There are only two proper ways: Quoting or escaping. This PR did the latter and is what was tested, so I'm keeping this.

So: We are used to either (Extended) POSIX regular expression syntax (brackets and pipes escaped) or PCRE-style dialects (brackets and piped unescaped). What we are seeing here is a mixture, but it's not due to the regexp syntax itself (which seems PCRE-like), but rather due to the fact that the pattern has to go through the qmake syntax parser before being interpreted as a regexp.

As we now have a logical explanation, I'm merging this.

CHANGELOG: Code: Add make clang_format helper command to simplify code formatting for contributors.