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

Automate most of the 3C code style checks. #716

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mattmccutchen-cci
Copy link
Member

This is something we've said we've wanted for a long time. It isn't particularly urgent now, but for whatever reason (maybe because I was running clang-tidy on #657 recently), I was inspired to hack it together today.

This PR includes:

  1. Build targets to check for style violations (lint-3c, validate-3c, etc.) and a script to try to automatically fix them (clang/tools/3c/utils/code_style/fix-all.sh).
  2. A quick pass of fixing existing style issues in the 3C code, just so that we can test that the build targets in part (1) pass. Some of these style fixes may not be what we want, and we may do the fix pass over later.
  3. A draft documentation update stating that the automated checks should pass on main at all times (and thus, on every PR before it is merged). We can discuss when or if we actually want to adopt such a policy. Another option is to merge the tools but just have me run them periodically as a better automated version of what I've been doing for the omnibus PRs to Microsoft.

I'm going ahead and marking this PR ready for review of parts (1) and (3). Obviously, a style fix pass on main will be disruptive to our 3C branches in progress, so we can discuss when would be a good time to actually do it. It should be harmless to go ahead and merge part (1) by itself once we are satisfied with the quality of that code; if we want to proceed that way, I can move parts (2) and (3) to another PR and merge this one (after review), or move part (1) to another PR and merge that one.

This commit includes a draft documentation update stating that the
automated checks should pass on `main` at all times. This documentation
is included for review; we can discuss when (or if) we want to actually
impose such a policy.
This is just so that we can test that `lint-3c` is implemented correctly
and passes. Some of these fixes may not be what we want, and we may do
the fix pass over later.
@aaronjeline aaronjeline self-requested a review September 29, 2021 18:17
@mattmccutchen-cci
Copy link
Member Author

Decision from the meeting today: We probably do want to eventually make a policy of requiring the automated style checks to pass on every PR, but we want to gain some experience with the tools first. So the plan:

  • Wait until our large branches in progress (soon to be represented by labeled PRs) are merged, then review and merge parts (1) and (2).
  • "Soft launch": PR authors are encouraged but not required to run the checks and are encouraged to report any problems with the tools.
  • Once we are confident enough in the tools, impose the policy (part (3)).

We're interested in posting the status of all checks that our policy requires for each PR onto the PR as GitHub status checks, though our current setup with the private actions repository makes this nontrivial to implement. Aaron urges that if/when we post the status checks, we find a way to display the status of the regression tests and the style checks separately, e.g., as separate GitHub status checks. Implementation details TBD.

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.

1 participant