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

Enforce OpenSSL coding style #21468

Closed
wants to merge 3 commits into from
Closed

Enforce OpenSSL coding style #21468

wants to merge 3 commits into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Jul 16, 2023 •
edited

Issues found by running the checkpatch.pl Linux script to enforce OpenSSL coding style.

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

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jul 16, 2023
slontis
slontis reviewed Jul 17, 2023
View reviewed changes
engines/e_capi_err.c Outdated Show resolved Hide resolved
@slontis
Copy link
Member

slontis commented Jul 17, 2023

Is the codespell commit supposed to be here? there is another PR for this?

t8m
t8m requested changes Jul 17, 2023
View reviewed changes
crypto/context.c Outdated Show resolved Hide resolved
include/internal/recordmethod.h Outdated Show resolved Hide resolved
@DimitriPapadopoulos
Copy link
Contributor Author

The codespell commit indeed doesn't belong here – removed.

@t8m t8m 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: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly tests: exempted The PR is exempt from requirements for testing labels Jul 17, 2023
@t8m
Copy link
Member

t8m commented Jul 17, 2023

Is this no longer draft now?

@DimitriPapadopoulos
Copy link
Contributor Author

Still a draft, I need more time to apply other checkpatch.pl suggestions. I am trying to organise them using one commit for each error class.

@slontis
Copy link
Member

slontis commented Aug 4, 2023

There is also nothing stopping you doing multiple PRs.

@DimitriPapadopoulos
Remove repeated words
dffc74c 
Found by running the checkpatch.pl Linux script to enforce coding style.
@DimitriPapadopoulos
"foo * bar" should be "foo *bar"
ddc5c59 
Found by running the checkpatch.pl Linux script to enforce coding style.
@DimitriPapadopoulos
Bad function definition
a51fb12 
void f() should probably be void f(void)

Found by running the checkpatch.pl Linux script to enforce coding style.
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review August 7, 2023 05:44
@DimitriPapadopoulos
Copy link
Contributor Author

@slontis This is the first PR then 😄

@slontis
Copy link
Member

slontis commented Aug 7, 2023

Please consider using fixup commits rather than force pushes, once you are at the review fixup stage..

git commit --fixup commitid

Where commitid is obtained from a previous commit using

git log

This makes it easier to review what has changed since last commit.. (looking at > 50 files)

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Sep 7, 2023
@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 Sep 8, 2023
@DimitriPapadopoulos
Copy link
Contributor Author

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

Does the above mean I have to somehow update this branch for @openssl/otc?

@t8m
Copy link
Member

t8m commented Sep 8, 2023

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

Does the above mean I have to somehow update this branch for @openssl/otc?

Nope. No action needed on your side.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m
Copy link
Member

t8m commented Sep 11, 2023

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Sep 11, 2023
openssl-machine pushed a commit that referenced this pull request Sep 11, 2023
@DimitriPapadopoulos @t8m
Remove repeated words
ad31628 
Found by running the checkpatch.pl Linux script to enforce coding style.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21468)
openssl-machine pushed a commit that referenced this pull request Sep 11, 2023
@DimitriPapadopoulos @t8m
"foo * bar" should be "foo *bar"
bbaeadb 
Found by running the checkpatch.pl Linux script to enforce coding style.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21468)
openssl-machine pushed a commit that referenced this pull request Sep 11, 2023
@DimitriPapadopoulos @t8m
Bad function definition
e22ebb8 
void f() should probably be void f(void)

Found by running the checkpatch.pl Linux script to enforce coding style.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21468)
@DimitriPapadopoulos DimitriPapadopoulos deleted the checkpatch branch September 11, 2023 08:47
@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Sep 13, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DimitriPapadopoulos @slontis @t8m @openssl-machine @paulidale