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: fix detection of missing/extra blank lines in local decls #18789

Closed
wants to merge 3 commits into from
Closed

check-format.pl: fix detection of missing/extra blank lines in local decls #18789

wants to merge 3 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Jul 12, 2022 •
edited

This fixes issues like the one reported in #17172 (comment).

Also

  • fix false positive on for(;; stmt)
  • improve wording: 'no' -> 'missing'
  • further minor improvements

@DDvO DDvO added branch: master Merge to master branch 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 labels Jul 12, 2022
@DDvO DDvO requested a review from levitte July 12, 2022 22:20
@DDvO DDvO force-pushed the check-format_fix_decls-blank-line+wording branch from bc619ea to a5d5140 Compare July 12, 2022 23:06
@DDvO DDvO mentioned this pull request Jul 12, 2022
Closed
2 tasks
@DDvO DDvO requested a review from paulidale July 13, 2022 09:25
@DDvO
Copy link
Contributor Author

DDvO commented Jul 13, 2022 •
edited

As for all content-wise changes to this tool, I'd not really expect reviewers to understand and check the details of fiddling with the complex regular expressions etc.

A simple-to-use quality check is to run

util/check-format.pl util/check-format-test-positives.c 
util/check-format.pl util/check-format-test-negatives.c

which are supposed to not produce any output.

  • For the positive test cases this means that all inserted coding style flaws are detected.
  • For the negative test cases this means that no false positives are reported for the given code.

@levitte levitte removed the approval: otc review pending This pull request needs review by an OTC member label Jul 13, 2022
@DDvO
Copy link
Contributor Author

DDvO commented Jul 15, 2022 •
edited

@paulidale can you please approve this one as well?

@paulidale paulidale 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 Jul 19, 2022
@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 Jul 20, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@DDvO @hlandau
check-format.pl: improve wording: 'no' -> 'missing'; further minor im…
6326314 
…provements

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18789)
openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@DDvO @hlandau
check-format.pl: fix false positive on 'for(;; stmt)'
4e9fa07 
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18789)
openssl-machine pushed a commit that referenced this pull request Jul 20, 2022
@DDvO @hlandau
check-format.pl: fix detection of missing/extra blank lines in local …
23757b6 
…decls

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18789)
@hlandau
Copy link
Member

hlandau commented Jul 20, 2022

Merged to master. Thank you.

This did not merge cleanly to 3.0, do you want to open another PR for it?

@hlandau hlandau closed this Jul 20, 2022
@DDvO
Copy link
Contributor Author

DDvO commented Jul 20, 2022

Merged to master. Thank you.

I'm pleased to provide fixes like these.

This did not merge cleanly to 3.0, do you want to open another PR for it?

I'll do - likely I forgot backporting some previous fix of the tool to 3.0.

DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: improve wording: 'no' -> 'missing'; further minor im…
21caa6b 
…provements

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: fix false positive on 'for(;; stmt)'
586c64c 
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: fix detection of missing/extra blank lines in local …
ace11a9 
…decls

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: improve wording: 'no' -> 'missing'; further minor im…
2ba8446 
…provements

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Cherry-picked from openssl#18789)
DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: fix false positive on 'for(;; stmt)'
9567181 
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Cherry-picked from openssl#18789)
DDvO added a commit to siemens/openssl that referenced this pull request Jul 20, 2022
@DDvO
check-format.pl: fix detection of missing/extra blank lines in local …
7b212bd 
…decls

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Cherry-picked from openssl#18789)
@DDvO
Copy link
Contributor Author

DDvO commented Jul 20, 2022

After cherry-picking the other recent fixes to check-format.pl to 3.0,
cherry-picking the commits of this PR to 3.0 succeeded cleanly.

sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
check-format.pl: improve wording: 'no' -> 'missing'; further minor im…
7cd580e 
…provements

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
check-format.pl: fix false positive on 'for(;; stmt)'
d31b9f4 
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@DDvO @sftcd
check-format.pl: fix detection of missing/extra blank lines in local …
2e68fd6 
…decls

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#18789)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch 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
5 participants
@DDvO @openssl-machine @hlandau @levitte @paulidale