Make sure that everything is Clang formatted #480
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This new script has several advantages over the old one. First, it’s directly executable. The previous script did not use a shebang, so users would have to invoke “sh tools/formatter.sh”. This new script does include a shebang, so users can just run “tools/formatter.sh”. Second, it’s available under the same license as the rest of the repo. The previous version of the script was released under the MIT License, but since it’s a part of a larger GPL’d work, users would potentially have to comply with two licenses at the same time. This new script is available under the same license as the rest of the repo in order to make the licensing situation simpler. Third, it’s a little bit easier to read. The old script put globs for every possible file extension on a single line, and escaped them in an unusual way. Additionally, that line in the previous script contained more than just globs. It also contained the find command and the start of a for loop. This new script stores the globs in a variable in order to make the code easier to read by decreasing the amount of stuff that’s happening on a single line. The new script also makes things easier to read by using single quotes to escape globs instead multiple backslashes. It also makes things easier to read by decreasing the total number of globs (See the final point for details). Fourth, this new script is more lenient when it comes to the user’s current working directory. The previous script would only work properly if the user’s current working directory was the root directory of the Descent3 repo. This new script will work properly if the user’s current working directory is any directory that’s part of the Descent3 repo. Fifth, this new script can handle more filenames. The previous script would fail if characters like spaces appeared in the names of files. This new script will work even if filenames contain spaces, newlines, etc. There’s no files in the Descent3 repo that currently use those characters in their names, but it’s possible that there will be in the future. Sixth, this new script will ignore the same files that Git ignores. The previous script would process all files, even if they were ignored by Git. Seventh, this new script has slightly better code style. The previous script used tabs for indentation. This new script uses two spaces for indentation, just like .editorconfig says we should use. The previous script used semicolons at the end of lines. This new version doesn’t because semicolons are unnecessary in shell scripts. Eighth, this new script avoids truncating files. Apparently, part of the old script was truncating files [1]. Ninth, this new script is agnostic to the version of clang-format that’s installed on your system. The previous script required clang-format-16 in particular. Tenth, this new script respects the style settings in .clang-format. The previous script used “--style=LLVM” which prevents .clang-format from being loaded [2]. Finally, this new script is faster than the old one. For one thing, this new script doesn’t waste time looking for files that don’t exist. For example, the previous script would look for .java files because clang-format can format code written in Java [2]. This new script doesn’t bother looking for .java files because there aren’t any in the Descent3 repo. More importantly, the old script would run clang-format serially. This new script runs many instances of clang-format in parallel. There is a potential disadvantage to this script. It adds additional dependencies. Specifically, this new script depends on /bin/sh and git. That being said, users are highly likely to have both of those installed anyway, so it’s probably not a disadvantage in practices. At the same time, this new script also removes a dependency (the find command), so the upsides of this change almost certainly outweigh the downsides. [1]: <DescentDevelopers#81> [2]: <https://clang.llvm.org/docs/ClangFormat.html#standalone-tool>
Before this commit, .github/workflows/clang-format-check.yml used version 4.11.0 of jidicula/clang-format-action but used version 18 of clang-format. That was a mistake. That version of jidicula/clang-format-action doesn’t support version 18 of clang-format. Support for version 18 of clang format wasn’t added until version v4.12.0 of jidicula/clang-format-action [1]. [1]: <https://github.com/jidicula/clang-format-action/releases/tag/v4.12.0>
This reverts commit 97a22f5. The changes from that commit were only meant to be temporary, anyway. The commit message itself says “Change it to manual use for the time being.” The commit message doesn’t indicate when the clang-format Check workflow should be changed to automatic again, so now seems like as good a time as any.
This makes it easier to test branches before they get submitted as a PR. Additionally, the Descent 3 Build workflow can already be manually dispatched, so this change makes our GitHub Actions more consistent with each other.
This ensures that everything uses the format rules specified in .clang-format. Unfortunately, in order to make sure that the clang-format Check workflow succeeded, I had to make sure that my system had the same version of clang-format that jidicula/clang-format-action uses. I’m not sure what the best way to to run the script with an outdated version of clang-format is, but here’s how I was able to do it: 1. Make sure that the Nix package manager (https://nix.dev) is installed. 2. Create a file named shell.nix. Put the following in it: let url = "https://github.com/NixOS/nixpkgs/archive/refs/heads/nixos-unstable.tar.gz"; tarball = builtins.fetchTarball url; pkgs = import tarball { }; in pkgs.mkShell { name = "shell-for-Descent3-clang-format"; packages = let customLlvmPackages = pkgs.llvmPackages_18.override { officialRelease = { version = "18.1.3"; sha256 = "saQGbpYd95JuudwLcdG80GL8YhadH7TUY1FC0o0ithY="; }; }; in [ customLlvmPackages.clang-tools ]; } 3. Run “nix-shell”. 4. Change directory to the root of the Descent3 repo. 5. Run “tools/formatter.sh”.
13 tasks
|
Closing, see comments in #450 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Type
Description
This PR does multiple
clang-formatrelated things:It revamps
tools/formatter.sh.It fixes and re-enables the
clang-formatworkflow.It makes sure that all files obey the style settings in
.clang-format.Related Issues
Closes #450.
Checklist
Additional Comments
This PR is meant to be an alternative to #450. Here are the differences between this PR and that other PR.
/bin/sh,gitandclang-format. The other PR’s shell script depends on/bin/sh,which,clang-format,xargs/parallelandfind.which. The shell script that the other PR introduces depends on a version ofwhichthat support the-sflag. GNUwhichdoes not support the-sflag.. I don’t know how common GNUwhichis compared to other implementations ofwhich, but I do know that GNUwhichis a required package on NixOS and a mandatory package on Fedora Workstation.gitorclang-format, so checking to see if they’re available is not necessary.Important
I’m not so sure that this PR should actually be merged. I have a concern that I wrote about here. Depending on how we decide to resolve that concern, I might end up canceling this PR.