Skip to content

Restore the .clang-format file and explain how to use it.#139

Closed
aknrdureegaesr wants to merge 7 commits into
masterfrom
reestablish-formatting
Closed

Restore the .clang-format file and explain how to use it.#139
aknrdureegaesr wants to merge 7 commits into
masterfrom
reestablish-formatting

Conversation

@aknrdureegaesr

Copy link
Copy Markdown
Collaborator

With 4a7a955, the .clang-format file was removed, I presume, accidentally.

As part of the effort for #138 , this restores that file and also explains how to use it.

@aknrdureegaesr aknrdureegaesr marked this pull request as draft January 16, 2026 11:34
@aknrdureegaesr

Copy link
Copy Markdown
Collaborator Author

As I found out the hard way and @Chris-AC9KH also mentioned in #141 (comment) :

clang-format will sometimes reformat files fed to it even if the file hasn't been edited

So the recommendation to run clang-format-18 on the entire code base other than check-only mode needs to be removed.

We should do this only once, to establish a baseline that's clean, as far as coding style is concerned.

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

clang-format has already been run on all files except the JS8 checker files. This was done during the rebuild of the code tree, along with fixing several problems I found in the code.

I don't know how I can make this any more clear - you are using the wrong format for your clang-format implementation. Nobody uses WebKit on C++ code, why are you trying to do that? Do you not understand how clang-format works? There is over 30 different implementations of it in Xcode alone.

So I'm not really sure what you're trying to do here, and I don't think you know yourself. If you wanted to take part in it you should've showed up when the code tree was being rebuilt. But you're late to the party now.

I got better things to do than deal with old news that's trying to re-invent the wheel. So I'm going to state this here once and then I'm done with it:
fix your .clang-format file to use the LLVM standard that is already in place. Then put it in the tools directory so linux folks who don't have the suite of tools we have on MacOS can use it. Put clear developer documentation in the guide. But be very careful and make sure you know what you're doing because you NEVER run clang-format on an entire codebase. Full stop.

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

@wmiler @aknrdureegaesr @rruchte there's a LLVM standard clang-format configuration file in the tools directory in this branch, generated from Xcode. You can use it to get rid of that cobbled up WebKit one you're using, that is trying to implement some new coding style never seen before by the human race.
https://github.com/Chris-AC9KH/JS8Call-improved/tree/clang-format

Tested on MacOS with Xcode 26.0.1, and Windows 11 with Visual Studio 2024 it doesn't attempt to reformat any of the current codebase that has already been formatted. I don't know if linux will have the same results because linux does not use the same clang-format as MacOS and Windows. Maybe it will, maybe it won't. But you can try it.

If it works on linux, then you can put it in the tools directory for linux developers to use. And include your developer documentation on how to use it in the developer guide. I suggest using it on ONLY one file at a time, whatever is being worked on by the developer. NEVER apply it to the entire codebase. Add .clang-format to .gitignore so linux developers can copy it to the root of the file tree and then call it with:
clang-format -i -style=file JS8_xxx/filename.cpp (or whatever).

Adding it to .gitignore will prevent it from getting into the root of the source tree when linux developers submit PR's. MacOS and Windows developers don't need it since these tools are built into their IDE's, and both Mac and Windows use the same LLVM standard if you select that in their respective IDE's. They don't need any .clang-format configuration file.

This should fix your code formatting standard issue (assuming it works on linux) without reformatting the codebase (again). From that point it can be looked at on a per-PR basis to see if has been used pre-PR.

@aknrdureegaesr

Copy link
Copy Markdown
Collaborator Author

You can use it to get rid of that cobbled up WebKit one you're using, that is trying to implement some new coding style never seen before by the human race.

😆

Thanks for any appreciation of the work @wmiler and I put into it.

Seriously: It is more important to me we use a uniform coding style in the first place than which coding style that is.

I see no reason to tug that file into a subdirectory, rather than the project's root directory where clang-format will look for it without special instructions. Do you?

@aknrdureegaesr

Copy link
Copy Markdown
Collaborator Author

Add .clang-format to .gitignore so linux developers can copy it to the root of the file tree and then call it with:
clang-format -i -style=file JS8_xxx/filename.cpp (or whatever).

Would the existence of that file in the root directory hurt Windows or Mac people?

@aknrdureegaesr

aknrdureegaesr commented Jan 19, 2026

Copy link
Copy Markdown
Collaborator Author

But you can try it.

Ok, in the process. First result: The Debian Trixie clang-format-18 doesn't like that file. To see what the options are, my PC presently compiles clang-format from source, their branch origin/main. (I like that branch name.)

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

The first .clang-format file was half WebKit, half something else. It didn't meet any standard I've ever seen.

So, what problem does Debian trixie have with the standard LLVM format? It can't do LLVM standard? So in other words, if you run clang-format on Linux it won't work without a .clang-format file? So you can't use clang-format -style=LLVM filename.cpp and get it to work without that file? If that doesn't work on Debian trixie then whatever clang-format you got is seriously broken.

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

Would the existence of that file in the root directory hurt Windows or Mac people?

The file doesn't need to be in the root of the source tree or you're gonna format every file in the system with it. I'm trying to figure out why you think you need a .clang-format file in the first place? If using clang-format at the MacOS command line you use:

clang-format -i --style="{BasedOnStyle: LLVM, IndentWidth: 4}" <filename.cpp>

That command line command is the same one Xcode uses in the IDE. You don't use any .clang-format file. If I want the indenting to be 2 space instead of 4, I just click on that on the Xcode configuration like so:

Screenshot 2026-01-19 at 19 46 05

So if you want clang-format to use 2 space indenting at the command line you use:

clang-format -i --style="{BasedOnStyle: LLVM, IndentWidth: 2}" <filename.cpp>

If I'm working with any of the MacOS WebKit code then Xcode detects that automatically and switches to WebKit. But at the command line I would use:

clang-format -i --style="{BasedOnStyle: WebKit, IndentWidth: 2}" <filename.cpp>

I think you get the idea. But you don't need any sort of .clang-format file unless you are defining some custom version of one of the code formatting standards. So like I said, put it in the tools directory as a reference or tool if somebody doesn't know how to use clang-format. But otherwise I been using it for 19 years and have never used a .clang-format file yet.

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

@aknrdureegaesr I also fail to understand why you are using clang-format 18. clang-format 21 is in Xcode 26.0.1 and that will be updated to clang-format 23 at the next Xcode point release. clang-format 18 is seriously out-of-date and does not support some of the newer C++ standards that have come out since it was released.

@wmiler

wmiler commented Jan 20, 2026

Copy link
Copy Markdown
Collaborator

@aknrdureegaesr I also fail to understand why you are using clang-format 18. clang-format 21 is in Xcode 26.0.1 and that will be updated to clang-format 23 at the next Xcode point release. clang-format 18 is seriously out-of-date and does not support some of the newer C++ standards that have come out since it was released.

clang-format 20 is the latest available in the Ubuntu 24.04 repos. clang 23 isn't released anywhere yet. clang-18 is the default version. Just because you can be on the "latest and greatest", doesn't mean you should be.

@wmiler

wmiler commented Jan 20, 2026

Copy link
Copy Markdown
Collaborator

So if you want clang-format to use 2 space indenting at the command line you use:

clang-format -i --style="{BasedOnStyle: LLVM, IndentWidth: 2}" <filename.cpp>

This shows no issues with any of the cpp files. Looks like you would need to do some work on JS8_Include/commons.h tho, haha. Oh, and JS8_Main/JS8MessageBox.h but that's just one line.

Chris's clang-format file doesn't work for me tho, it's throwing a lot of this'n'that missing.

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

I used four-space indent when I did the recent code overhaul.

But anyway, that goes to show that clang-format is not the same between the linux versions and what MacOS and Windows uses. You can run clang-format --dump-config with your desired style and generate a .clang-format file, which is what I did with clang-format 21 in Xcode 26. If that file doesn't work with linux, then linux clang and MacOS/Window clang are not compatible. Which makes sense because the clang compiler won't build JS8Call on linux, but it does on both MacOS and Windows.

@wmiler wmiler self-requested a review January 21, 2026 12:55
@wmiler

wmiler commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

Might not have v20 in the Ubuntu 22.04 repos. Which is what we use on the CI runs. And, NO, you may not change it to ubuntu-latest (or-24.04) runner, because that will totally break AppImage generation.

@aknrdureegaesr

Copy link
Copy Markdown
Collaborator Author

The 2022 Ubuntu we use has this to offer:

$ docker run --rm -ti ubuntu:22.04
root@4abf3337421b:/# apt-get update
...
root@4abf3337421b:/# apt-cache search clang-format
ament-cmake-clang-format - CMake build system for ROS 2 ament packages (clang_format)
arcanist-clang-format-linter - clang-format linter for Arcanist
clang-format - Tool to format C/C++/Obj-C code
clang-format-11 - Tool to format C/C++/Obj-C code
clang-format-12 - Tool to format C/C++/Obj-C code
clang-format-13 - Tool to format C/C++/Obj-C code
clang-format-14 - Tool to format C/C++/Obj-C code
python3-ament-clang-format - Python 3 module for clang_format support in ROS 2 ament packages
clang-format-15 - Tool to format C/C++/Obj-C code

We could potentially use a different OS just for code checking. Before we go there, let's see how well clang-format-15 does.

@wmiler

wmiler commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

Yeah, it was a really long time ago! I can whip up a dry-run runner over on my repo and give it a whirl there. And it would actually make more sense as a separate runner for tooling-related runs anyway.

@wmiler

wmiler commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

@aknrdureegaesr Andreas, this is the way you want to do this, as a separate yaml file.
Please revert your changes to ci-linux.yml, you are polluting the Actions list here.

test-clang-format.yml

name: Format check on Linux Ubuntu
run-name: ${{ (github.event.pull_request.title) || 'CI' }} Format check on Linux Ubuntu

on:
  [pull_request]

jobs:
  clang_format_check:
    runs-on: ubuntu-latest
    name: Check Code Style
    strategy:
      fail-fast: false

    steps:
    - uses: actions/checkout@v5
    
    - name: Install minimum required dependencies (Ubuntu)
      run: |
        sudo apt-get update
        sudo apt-get install -y clang-format-20
    - name: Run clang-format dry-run
      run: |
        clang-format-20 --dry-run --style='{BasedOnStyle: LLVM, IndentWidth: 4}' JS8*/*.h JS8*/*.cpp

@wmiler

wmiler commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

@Chris-AC9KH

Copy link
Copy Markdown
Collaborator

I don't recall that I ever ran clang-format on those JS8_include files unless it was modified by the recent code changes to mainwindow.cpp and the doxy hooks. If it wasn't touched by that the file was likely not formatted.

@Chris-AC9KH

Chris-AC9KH commented Mar 10, 2026

Copy link
Copy Markdown
Collaborator

I'm marking this stale because of several reasons:

  • there's no consistency in clang-format between platforms
  • I strongly object to using any sort of automated code formatting because different coders have different styles
  • we started from a baseline after New Year's
  • all new commits are reviewed before merge, and if there is some glaring flaw in formatting it can be pointed out and fixed at merge time

As long as the code remains readable and well-written, there is no need to enforce any sort of universal formatting stye, as it does nothing but create friction among developers

@wmiler wmiler added the wontfix This will not be worked on label Apr 30, 2026
@wmiler

wmiler commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Closing this PR as wontfix.

@wmiler wmiler closed this Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge stale wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants