Running clang-format on github does not result in no changes
- Dominant language
- C++
- Stars
- 1.9k
- Forks
- 283
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 135
Description
When running `clang-format` (as provided from the packaged tool-chain), this results in a few changes, which will generate unnecessary churn if contributors want to provide a clean PR and run clang-format before and accidentally verschlimmbesser the diff.
To reproduce:
```bash
CLANG_FORMAT=$(bazel run -c opt --run_under="echo" @llvm_toolchain//:clang-format 2>/dev/null)
${CLANG_FORMAT} -i $(find xls -name "*.h" -o -name "*.cc")
```
Afterwards `git diff` does not result in an empty diff.
Observations and possible mitigation
* Header ordering changes. Reason is that the path prefixes are possibly different in the copybara roundtrip. Possible **Mitigation**: run `clang-format` as last step when copy code in copybara (something like that is already done with `buildifier`).
* Difference in formatting: in constructors, an empty `{}` body is formatted with space in front coming from the git repo, but without space in front after `clang-format`. From what I can tell, this is due to the llvm version. XLS [uses toolchain 17.0.6](https://github.com/google/xls/blob/2037a514041ea1f7279340ced0df0e9aa42e985b/MODULE.bazel#L11-L13), I think this different formatting is done with newer clang formats. Possible **Mitigation** Update version in used toolchain.
* Right hugging pointers and reference symbols are formatted left-hugging (`int *a` -> `int* a`) according to [the configuration](https://github.com/google/xls/blob/2037a514041ea1f7279340ced0df0e9aa42e985b/.clang-format#L6-L8) but there are a few files in the repo where these symbols were right-aligned and subsequently fixed with the clang-format run. I suspect the special handling for these symbols is not set in the internal repo configuration for clang-format and `DerivePointerAlignment` is true there. **Mitigation** Use same clang-format configuration internally.
Contributor guide
Assessment
This issue has not been assessed yet.