Skip to content

Fix #11073 - Bump clang-format from 10 to 19#11082

Merged
Myoldmopar merged 3 commits intodevelopfrom
11073_Bump_Clang_Format_squash
May 23, 2025
Merged

Fix #11073 - Bump clang-format from 10 to 19#11082
Myoldmopar merged 3 commits intodevelopfrom
11073_Bump_Clang_Format_squash

Conversation

@jmarrec
Copy link
Contributor

@jmarrec jmarrec commented May 23, 2025

Pull request overview

  • This is a squash of Fix #11073 - Bump clang-format from 10 to 19 #11081

  • Fixes Clang Format woefully out of date #11073

  • I initially bumped to 18, which is what Ubuntu has. But Latest MSVC using clang-format 19.1.5, and this is harder for windows users to install a specific version

  • Issue was that there was formatting differences (minor) being 18 and 19, but you CANNOT configure that (it's the internal parser), so for these I basically did a bit of refactoring where possible, and I turned off clang format for that (it's like 2 lines in src/ and 3 in tst/)

  • There are 0 formatting differences in clang-format-18, 19 and 20 right now!

  • I have redone the clang format action to move away from jidicula to a custom one

  • I ran clang-tidy-18 with --checks=-*,readability-braces-around-statements, and I enabled the InsertBraces: true in clang-format as discussed with @Myoldmopar previously

Description of the purpose of this PR

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

jmarrec added 3 commits May 23, 2025 12:01
@jmarrec
Update clang-format rules and CI workflow
8044053 
Main change is bumping to clang-format-19, enabling InsertBraces: true.

For the CI action, replace jidicula with custom action
Switches from using the `jidicula/clang-format-action` to a custom, in-house implementation for running clang-format.

Benefit:
* Only run for changed files for a Pull Request, NOT the entire code base (it takes 4min to run for src/ and tst/ and both will individually spin a new docker container)
* Run clang-format in parallel, not serially like the jidicula action does
* For a PR: uploads a patch diff that can be applied locally
* For a push to develop, this will AUTOCORRECT + do another commit that adds it to .git-blame-ignore-revs
* I'm providing clearer logs + a github step summary markdown table
f4d009
@jmarrec
Apply clang-format-19 and clang-tidy --checks=-*,readability-braces-a…
9985a5f 
…round-statements
@jmarrec
Add previous commit to .git-blame-ignore-revs
232a688
@jmarrec jmarrec self-assigned this May 23, 2025
@jmarrec jmarrec added Defect Includes code to repair a defect in EnergyPlus Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) labels May 23, 2025
@jmarrec jmarrec mentioned this pull request May 23, 2025
Closed
20 tasks
@jmarrec jmarrec requested a review from Myoldmopar May 23, 2025 10:06
@Myoldmopar
Copy link
Member

Myoldmopar commented May 23, 2025

Alright this will be great. It will be so much easier for our team since it will be an updated version of Clang Format. Visual Studio should do a better job of auto-formatting. No more having to track down old versions of Clang Format.

And yes, it does run so much faster now, thanks @jmarrec!

I understand this will cause some conflicts, but they should be very straightforward. Spacing/line breaks, basic formatting changes. I anticipate being to remedy the conflicts on each branch as I'm reviewing today. If I get stuck, I'll just checkout the branch version of the file and then apply updated Clang format locally. Off I go. Thanks @jmarrec !

@Myoldmopar Myoldmopar merged commit c94e83c into develop May 23, 2025
11 checks passed
@Myoldmopar Myoldmopar deleted the 11073_Bump_Clang_Format_squash branch May 23, 2025 14:10
@jmarrec jmarrec mentioned this pull request Oct 2, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Myoldmopar Myoldmopar Awaiting requested review from Myoldmopar

Assignees

@jmarrec jmarrec

Labels

Defect Includes code to repair a defect in EnergyPlus Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clang Format woefully out of date

4 participants

@jmarrec @Myoldmopar @nrel-bot-2 @nrel-bot-2c