Skip to content

Comments

Apply non-time-sensitive prerequisites for clang-format to HEAD#29241

Closed
bob-beck wants to merge 8 commits intoopenssl:masterfrom
bob-beck:pre-clang-format-anytime
Closed

Apply non-time-sensitive prerequisites for clang-format to HEAD#29241
bob-beck wants to merge 8 commits intoopenssl:masterfrom
bob-beck:pre-clang-format-anytime

Conversation

@bob-beck
Copy link
Contributor

@bob-beck bob-beck commented Nov 27, 2025

This does the preparatory work for HEAD to prepare for clang-format.

The equivalent to this will need to get done for all the branches as well.

This is not sensitive to the repository being frozen, as it does not yet make format changes to any file. All this does is add the clang-format file, and clean up some of our things that get in the way of reformatting the source code.

For 1657

And can be done across branches to reduce the day-of-reformat load.

Checklist
  • documentation is added or updated
  • tests are added or updated

@bob-beck bob-beck force-pushed the pre-clang-format-anytime branch from 804e45a to cfc4fb2 Compare November 27, 2025 19:56
@bob-beck
Copy link
Contributor Author

the style in some of these is wonky (as they themselves has been through many clang-formats) - I'm mostly ignoring this as clang-format will simply whack them all once applied anyway.

@bob-beck bob-beck requested review from Sashan, esyr and levitte November 27, 2025 19:58
@bob-beck bob-beck marked this pull request as ready for review November 27, 2025 19:58
@bob-beck bob-beck requested a review from nhorman November 27, 2025 20:00
@bob-beck bob-beck marked this pull request as draft November 27, 2025 20:01
@bob-beck
Copy link
Contributor Author

Bah.. gotta fix one of them. wait one.

@bob-beck bob-beck force-pushed the pre-clang-format-anytime branch from cfc4fb2 to b00777b Compare November 27, 2025 20:22
@bob-beck bob-beck added the style: waived exempted from style checks label Nov 27, 2025
@bob-beck bob-beck force-pushed the pre-clang-format-anytime branch from 7d466ba to 4c98f8e Compare November 27, 2025 20:33
@bob-beck
Copy link
Contributor Author

Hadn't run into CI where OSSL_LINE fails to set, needed to account for it. now fixed.

@bob-beck bob-beck marked this pull request as ready for review November 27, 2025 20:55
@bob-beck bob-beck force-pushed the pre-clang-format-anytime branch from 4c98f8e to 6ede47b Compare November 27, 2025 21:23
@bob-beck bob-beck requested a review from mattcaswell November 27, 2025 21:53
@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug triaged: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing labels Nov 28, 2025
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

makes sense. I don't insist on my proposal I just think it's worth to consider.

Copy link
Contributor

@nhorman nhorman left a comment

Choose a reason for hiding this comment

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

This looks fine to me once @t8m and @Sashan get their comments/questions addressed.

Also, should there be a corollary PR in https://github.com/openssl/technical-policies to replace the coding style document with some language that points people to the use of the clang-format file?

@bob-beck
Copy link
Contributor Author

This looks fine to me once @t8m and @Sashan get their comments/questions addressed.

Also, should there be a corollary PR in https://github.com/openssl/technical-policies to replace the coding style document with some language that points people to the use of the clang-format file?

I have those changes in a seperate diff (it was included in the original clang-format show people what will happen stacks if you go back and look at those, and I solicited feedback on them at the time)

Since I expect that to involve bikeshedding and delay, I have not included that in these stacks as they will be replicated on all branches, and are needed to get the things done. I will land the STYLE change separately.

@bob-beck bob-beck force-pushed the pre-clang-format-anytime branch from cc29500 to 43b38a3 Compare November 28, 2025 18:36
@bob-beck bob-beck requested review from Sashan, nhorman and t8m November 28, 2025 18:37
@bob-beck bob-beck requested a review from Sashan December 1, 2025 17:20
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

looks good to me. thanks.

@Sashan Sashan added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 1, 2025
@bob-beck bob-beck added the extended tests Run extended tests in CI label Dec 2, 2025
@bob-beck bob-beck closed this Dec 2, 2025
@bob-beck bob-beck reopened this Dec 2, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 2, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Dec 3, 2025

Merged to the master branch. Thank you.

@t8m t8m closed this Dec 3, 2025
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
In particular fix the regex magic to be tolerant of different ways
of formatting a main program.

My past life had forgotten this magic 14 years ago when we converted
it to just a table of commands in the forks.

https://www.youtube.com/watch?v=mWbbjvYmN8A

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
(in it's final form it will work with either compiler
because it's currently one line, but was tripped up before
by the #ifdef, so redid it to be consistent with the
other changes previously in this stack)

While I am here correct the test to test for all possible
return values of ERR_get_error_all, without the #ifdefs

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Similar to the previous errtest.c fix this also is not broken
by any reformatting today, but this change makes this follow
the same pattern as the other things that test OPENSSL_LINE
after the fact so we maintain the same paradigm everywhere.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
They are not asm, but spit out stuff that is not C

Clang-format gets confused and does bad things with them.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
If OPENSSL_LINE ends up on a different line than the following call here,
this test breaks.

We should perhaps reconsider if testing the reporting of OPENSSL_LINE
is what we want in a malloc test, but that's for another time than now.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
we assume these to be order sensitive and not self contained, so
as per our new style we disable clang format around them.

we should consider renaming to .inc, or doing away with some
of these and just putting the code inline, but that's for
later consideration.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
In particular fix the regex magic to be tolerant of different ways
of formatting a main program.

My past life had forgotten this magic 14 years ago when we converted
it to just a table of commands in the forks.

https://www.youtube.com/watch?v=mWbbjvYmN8A

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
(in it's final form it will work with either compiler
because it's currently one line, but was tripped up before
by the #ifdef, so redid it to be consistent with the
other changes previously in this stack)

While I am here correct the test to test for all possible
return values of ERR_get_error_all, without the #ifdefs

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
Similar to the previous errtest.c fix this also is not broken
by any reformatting today, but this change makes this follow
the same pattern as the other things that test OPENSSL_LINE
after the fact so we maintain the same paradigm everywhere.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
They are not asm, but spit out stuff that is not C

Clang-format gets confused and does bad things with them.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
If OPENSSL_LINE ends up on a different line than the following call here,
this test breaks.

We should perhaps reconsider if testing the reporting of OPENSSL_LINE
is what we want in a malloc test, but that's for another time than now.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
we assume these to be order sensitive and not self contained, so
as per our new style we disable clang format around them.

we should consider renaming to .inc, or doing away with some
of these and just putting the code inline, but that's for
later consideration.

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29241)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch extended tests Run extended tests in CI style: waived exempted from style checks tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Freeze/Unfreeze tree and land clang-format changes Dec 9. Prepare WebKit reformat for master and all appropriate branches.

6 participants