Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cppwinrt code base should have clang-format style formatting enforced automatically #1461

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dmachaj
Copy link
Contributor

@dmachaj dmachaj commented Dec 6, 2024

Closes #1459

Why is this change being made?

Over time a lot of style inconsistencies have snuck into this code base (including things like tabs vs. spaces within a single file). These aren't in and of themselves a big deal but it is nice to apply tooling to this type of problem so that it becomes automatic and we don't have to worry about it during code reviews.

Briefly summarize what changed

This set of changes follows the pattern established by WIL in microsoft/wil#416, with some modifications. The .clang-format file in use here is a copy of the WIL file, with some light modifications to reduce the number of lines churned. Settings like "don't indent the first level of namespace" touched a massive fraction of lines of code so those were relaxed to more closely match the exsting patterns in this repo. I also decided to embed the "check formatting during CI" directly in the YML instead of having a separate script for it. I think that is specialized enough that it belongs in the YML whereas having multiple formatting batch files can be confusing.

The build_test_all.cmd script was modified to automatically run clang-format before building the code. I think most/all contributors rely on that script so that should make formatting automatic and therefore not something that needs to be worried about at all.

The PR build pipeline was also modified to run clang-format before building and will fail if any of the modified files has changes from formatting. This will insta-fail any PR builds that don't match formatting requirements. Therefore any PRs that build successfully meet the style and nobody has to review for style.

By far the biggest set of changes come from the formatting. Just about 100% of files have some sort of change made to them. Many files have tiny/trivial changes. Some had a lot of formatting changes. I tried to tweak the configuration a bit to reduce the churn as much as possible while still becoming consistent across the repo (see https://clang.llvm.org/docs/ClangFormatStyleOptions.html for the full list of options).

We can debate style choices as part of this PR to try and find the ideal balance between desired style and lines churned. I'm happy to tweak the .clang-format file based on feedback.

A very small set of locations had formatting disabled because I wasn't sure if it was doing the right thing. We can add more suppressions if necessary.

How was this change tested?

build_test_all.cmd locally.

@dmachaj dmachaj requested review from oldnewthing, kennykerr, jonwis, DefaultRyan and dunhor and removed request for oldnewthing December 6, 2024 21:32
.clang-format Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
cppwinrt/cmd_reader.h Outdated Show resolved Hide resolved
@@ -302,7 +291,7 @@ WINRT_EXPORT namespace winrt
}

template <typename T, std::enable_if_t<!std::is_base_of_v<Windows::Foundation::IUnknown, T>, int> = 0>
auto put_abi(T& object) noexcept
auto put_abi(T & object) noexcept
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening here? Earlier in this same file, it left these reference parameters left-aligned. But here it suddenly switches to middle alignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one. I tried setting ReferenceAlignment: Left and it still adds the space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the templated T is what confuses it. I see many examples of changes in this PR where var & i is correctly adjusted to be var& i. So this is not universally wrong. There are just a small number of false positive type matches.

@dmachaj dmachaj force-pushed the user/dmachaj/clang-format branch from fe1acfb to 7abe7b3 Compare December 9, 2024 20:43
@dmachaj dmachaj changed the title cppwinrt code base should have clang-tidy formatting enforced automatically cppwinrt code base should have clang-format sytle formatting enforced automatically Dec 12, 2024
@dmachaj dmachaj changed the title cppwinrt code base should have clang-format sytle formatting enforced automatically cppwinrt code base should have clang-format style formatting enforced automatically Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cppwinrt code base should have clang-tidy formatting enforced automatically
4 participants