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

apps.c: fix various coding style nits found by check-format.pl #17435

Closed
wants to merge 2 commits into from
Closed

apps.c: fix various coding style nits found by check-format.pl #17435

wants to merge 2 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Jan 6, 2022

Part of the nits were found by the enhancements to the tool in #17434.

@DDvO DDvO added approval: otc review pending This pull request needs review by an OTC member triaged: refactor The issue/pr requests/implements refactoring labels Jan 6, 2022
@DDvO DDvO mentioned this pull request Jan 6, 2022
Closed
@paulidale paulidale added approval: done This pull request has the required number of approvals branch: master Merge to master branch and removed approval: otc review pending This pull request needs review by an OTC member labels Jan 6, 2022
@DDvO
Copy link
Contributor Author

DDvO commented Jan 7, 2022 •
edited

Thanks @paulidale for quickly approving this

I meanwhile found that I have to rebase this PR,
and pondering on the remaining issues reported by check-format.pl,
I figured it would be good to fix also the remaining issues it reported,
which was not as easy as the ones in the first commit.
In particular, the tool warns on function bodies more than 200 lines long,
and indeed load_key_certs_crls() was more complex/lengthy than necessary, which I fixed.

Now check-format.pl as of #17434 is entirely happy with apps.c 😃

@DDvO DDvO requested a review from paulidale January 7, 2022 12:49
@paulidale
Copy link
Contributor

200 line functions won't be close to enough for many of the commands :(

paulidale
paulidale approved these changes Jan 7, 2022
View reviewed changes
apps/lib/apps.c
#define SET_EXPECT(val) \
(expect = expect < 0 ? (val) : (expect == (val) ? (val) : 0))
#define SET_EXPECT1(pvar, val) \
if ((pvar) != NULL) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

For an internal macro like this, we often dispense with the parenthesis around the arguments. Not a significant point and not a request to change anything.

@DDvO
Copy link
Contributor Author

DDvO commented Jan 7, 2022 •
edited

200 line functions won't be close to enough for many of the commands :(

I am very aware of this :(
Indeed, many of the source files in apps/*.c would deserve a major cleanup,
not only regarding their overly lengthy *_main() functions.

@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.

openssl-machine pushed a commit that referenced this pull request Jan 8, 2022
@DDvO
apps.c: fix various coding style nits found by check-format.pl
8cdb993 
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17435)
@DDvO
Copy link
Contributor Author

DDvO commented Jan 8, 2022

Merged - thanks @paulidale

@DDvO DDvO closed this Jan 8, 2022
@paulidale paulidale mentioned this pull request Jan 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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 triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DDvO @paulidale @openssl-machine