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.

Sign up for GitHub

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

Jump to bottom

check-format.pl: backport all recent improvements #20254

Closed
wants to merge 8 commits into from
Closed

check-format.pl: backport all recent improvements #20254

wants to merge 8 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Feb 9, 2023

This backports all recent improvements of the check-format tool (not only those in #19796) to 3.0 and 3.1,
as suggested by #19796 (comment).

@DDvO
check-format.pl: improve preprocessor directive handling; re-order st…
fcc74a2 
…ate variables

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18812)
@DDvO
check-format.pl: extend checking into macro bodies; small further imp…
3392331 
…rovements

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18812)
@DDvO
check-format.pl: report #if and #elif with constant condition; improv…
9500555 
…e checks on '/*'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18812)
@DDvO
check-format.pl: further fixes for whitespace reporting within 'for (…
b7e4d0a 
…...)'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18812)
@DDvO
check-format.pl: improve whitespace reporting on <op>=
7f5f2a7 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#18812)
@DDvO
check-format.pl: fix detection of function body start
5f9bfb4
@DDvO
check-format.pl: fix detection of '#ifdef __cplusplus'
07a868a
@DDvO
check-format.pl: fix statistics on whitespace and nesting issues
0dabed6
@DDvO DDvO added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present labels Feb 9, 2023
This was referenced Feb 9, 2023
Closed
Closed
@DDvO DDvO changed the title Check format backports check-format.pl: backport all recent improvements Feb 9, 2023
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Feb 12, 2023
@DDvO DDvO requested a review from hlandau February 16, 2023 16:38
@DDvO DDvO requested a review from paulidale February 24, 2023 15:06
@DDvO
Copy link
Contributor Author

DDvO commented Mar 12, 2023

Asking @openssl/committers for 2nd approval of this backport

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Mar 12, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM. I've checked that the 8 commits are the same as on master, and that the contents of the three modified files are also the same as on master

@tom-cosgrove-arm tom-cosgrove-arm 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 Mar 12, 2023
@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 Mar 13, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of function body start
e098291 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of '#ifdef __cplusplus'
856efc6 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix statistics on whitespace and nesting issues
4cd3bc4 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)
openssl-machine pushed a commit that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of function body start
a49ade2 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20254)
openssl-machine pushed a commit that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of '#ifdef __cplusplus'
d327f7c 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20254)
openssl-machine pushed a commit that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix statistics on whitespace and nesting issues
2bdc60c 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from #20254)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of function body start
2d86c5b 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit a49ade2)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of '#ifdef __cplusplus'
edf4e45 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit d327f7c)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix statistics on whitespace and nesting issues
ff190ae 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit 2bdc60c)
@DDvO
Copy link
Contributor Author

DDvO commented Mar 14, 2023

Merged/cherry-picked to 3.0 and 3.1 - thanks @t8m and @tom-cosgrove-arm for the approvals.

@tom-cosgrove-arm
Copy link
Contributor

@DDvO should this PR be closed now it's been merged?

@t8m t8m closed this Mar 14, 2023
@DDvO
Copy link
Contributor Author

DDvO commented Mar 14, 2023

@DDvO should this PR be closed now it's been merged?

Yes.
Yet usually I wait until the PR page lists the commits that have been merged, which usually takes a few minutes, and then close the PR.

DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of function body start
f11bd37 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit a49ade2)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix detection of '#ifdef __cplusplus'
a2a895f 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit d327f7c)
DDvO added a commit to siemens/openssl that referenced this pull request Mar 14, 2023
@DDvO
check-format.pl: fix statistics on whitespace and nesting issues
3868c77 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from openssl#20254)

(cherry picked from commit 2bdc60c)
@DDvO
Copy link
Contributor Author

DDvO commented Mar 14, 2023

This time only 8 such commits were listed before I had to leave for two hours,
and on return I found that still no more than 8 commits were shown,
although this PR constitutes 8 commits per versions 3.0 and 3.1, so 16 commits.

Then I realized that cherry-picking to 3.1 did not really happen in my upstream ref, so had to push again.
Now, for whatever reason, just three of the 8 new commits to 3.1 are meanwhile shown.
Still according to https://github.com/openssl/openssl/commits/openssl-3.1 all 8 commits made it in this time.

@tom-cosgrove-arm
Copy link
Contributor

Yes. Yet usually I wait until the PR page lists the commits that have been merged, which usually takes a few minutes, and then close the PR.

Thanks - as a new committer I want to make sure I understand the best practice here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@hlandau hlandau Awaiting requested review from hlandau

@paulidale paulidale Awaiting requested review from paulidale

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DDvO @openssl-machine @tom-cosgrove-arm @t8m