google / google/xls

Running clang-format on github does not result in no changes

Open
#2,006 0 comments 0 reactions 0 assignees View on GitHub
🧑‍🌾 gardener
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.