Can someone tests on Windows how it would behave there (does it work when clang-format is available, does it continue properly if it isn't)?.

I have tested and it does not work at the moment (when using the windows/deploy_windows.ps1 script). Here the result:

clang-format not in PATH:
Message in the output that clang-format failed.

clang-format in PATH:
Same message in the output that clang-format failed.

In both occassions, no harm on the build itself that completed without issue.

The reason for failing when clang-format is in the path, is because of the powershell script is executing the step from the "build" folder it creates.

Here a part of the clang-format command being executed:
clang-format -i windows/sound.cpp windows/ASIOSDK2/common/asio.cpp

This generates the message: no such file or directory (for each file)
This is because at this stage in the process the source and header files are still in the root project folder and not in the build folder.

Hope this helps.

I'm not sure I understand the benefit of this. Reformatting code with clang-format is not for a compiler's benefit; if it makes any difference to compilation, then that is a bug in clang-format.

Reformatting code is for the benefit of people reading and editing the code. So it makes sense for clang-format to be run on code to be committed, but not as an intermediate step in building.

Am I missing something?

I guess the idea was that a contributor would at some point make a local build and then 'silently' get pretty code.
So still makes sense to me, even the compiler won't care about how pretty he code is...

I guess the idea was that a contributor would at some point make a local build and then 'silently' get pretty code.
So still makes sense to me, even the compiler won't care about how pretty he code is...

Right, the idea is to make it simple for contributors to submit properly formatted code. Visual Studio will probably handle .clang-format integration, other editors (vim? emacs?) may not.

This PR is supposed to make it simpler in general.

clang-format should be used as a validation step only, in my view. We have a coding standard, it's required, PRs are rejected if they don't comply. clang-format automates this validation.

It should never make code changes.

So in that case there needs to be a make format or make clang-format that can be invoked optionally by the user to reformat.

If I clone or checkout a tree and then do a compilation, I wouldn't normally expect it to make any kind of changes to checked-out files, resulting in git status showing uncommitted files. (Notwithstanding the possibility of this happening to .qm files).

Visual Studio will probably handle .clang-format integration

In Visual Studio you just need to have the "C/C++" Extension installed while clang-format is installed on your system.
Then the C/C++ Extension uses clang-format out of the box with the style definition of the project

There is also the point that if every source file was unnecessarily rewritten to a formatted version of itself (even if identical), that will defeat the normal use of modification timestamps by make to only rebuild the files that have changed since the last make.

Disclaimer: I haven't yet studied the proposed changes, but am just talking generally, apologies if I have made a wrong assumption about how it will work.

What version of clang-format is required? I just tried to run it on my Pi and got the following:

tony@pi:~/jamulus $ clang-format src/*.h src/*.cpp
........
YAML:17:32: error: invalid boolean
AllowShortBlocksOnASingleLine: Empty
                               ^~~~~
Error reading /home/tony/jamulus/.clang-format: Invalid argument
YAML:17:32: error: invalid boolean
AllowShortBlocksOnASingleLine: Empty
                               ^~~~~
Error reading /home/tony/jamulus/.clang-format: Invalid argument
tony@pi:~/jamulus $ clang-format --version
clang-format version 7.0.1-8+rpi3+deb10u2 (tags/RELEASE_701/final)
tony@pi:~/jamulus $

Lots of the same error, one for each file.

What version of clang-format is required?

The workflow runs with version 10. I'm using 11 and it doesn't cause any errors/changes either.

Maybe those experiences (and the one by @dcorson-ticino-com in the other PR) may make us review the decision of not running the formatting in PRs itself... Hrm.

Maybe those experiences (and the one by @dcorson-ticino-com in the other PR) may make us review the decision of not running the formatting in PRs itself... Hrm.

I'm still uncomfortable with the idea of Github modifying stuff I have pushed to it, requiring me to pull back again. I feel it should either accept what I have given it, or reject it and expect me to fix it and resubmit.

What version of clang-format is required?

clang-format version 7.0.1-8+rpi3+deb10u2 (tags/RELEASE_701/final)```

Version 10 is required as it introduced some of the options we use (like AllowShortBlocksOnASingleLine: Empty).
I tried version 9 and it gives me the same errors as you wrote

Does anyone have an idea, which IDEs are mainly used?
I saw that integrating clang-format in Visual Studio is really easy and we could check if that's the case for other IDEs and document the required steps.
That could cover formatting for lots of contributions already. Then, for the contributors that don't use an IDE, a little more complicated way may be acceptable?!

I've only ever used Qt Creator for work on Qt code. Or vi. (Well, console vim, none of your fancy graphical gvim stuff.)

What's the status here?

• Will this get changed into a build (Makefile target) option?
• Or are we just going to close it?

At the moment, I type !clang to run clang-format locally (when I remember... and if my history still has it). Having to do make clang-format would be okay, but I'd not want it run automatically.

I've updated the source list file generation to be what it should be: A list of all .cpp and .h files, excluding Qt-auto-generated files and bundled libraries in libs/ and windows/nsProcess.
I have also added hints in the relevant places where included file extensions and excluded files are managed in order to ensure that both places will be updated.

It would be nice to see if this works properly on Mac and Windows as well.

@hoffie , I have downloaded your branch on my windows machine and do not see errors during the build process and the generated executable works fine. However I am not sure what to expect from the change made now. As I went into one of the files (main.cpp) and made modifications which would fail the format check (verified with the clang-format --dry-run option to check if the formatting errors would be detected and they were). However after running the build again, the file remains unchanged. If this is the correct behaviour, then I don't understand what the benefit is of this. I might be wrong....or might just not understand what is supposed to be happening.

@henkdegroot Thanks for confirming that the build works. As you assumed, the expectation is no longer that the build auto-fixes things. Instead, the scope is reduced to providing make clang_format as a convenience option for users with no clang IDE/editor integration. I had adapted the commit message, but I should obviously have clarified the PR title.

Thinking some more... As the scope of the PR has changed, changing title and PR content would not make sense regarding the existing comment history. I'm therefore closing this and have opened #2258 instead.