Style: ssl.h by FdaSilvaYY · Pull Request #4466 · openssl/openssl

FdaSilvaYY · 2017-10-04T23:01:31Z

fix some indents, and restrict to 80 cols some lines.

split from #4457 .

Checklist

documentation is added or updated
[N/A] tests are added or updated

kaduk

There are several preprocessor statements that are longer than 80 columns, but since it is not always possible to handle those nicely I did not look at them too closely.

kaduk · 2017-10-10T18:47:48Z

include/openssl/ssl.h

A style pass over ssl.h might reasonably be expected to normalize on one of ') (' or ')(' in function typedefs.
#2279 (comment) is perhaps in support of the latter (no space), but we've really been all over the place.

I would remove the space.

kaduk · 2017-10-10T18:49:31Z

include/openssl/ssl.h

',' is in column 81

better fixed with a typedef on call_proc type.

kaduk · 2017-10-10T18:51:50Z

include/openssl/ssl.h

Most of these deprecated version-specific methods have their comment extend past the 80th column

Not the most prominent style issue.

kaduk · 2017-10-10T18:52:19Z

include/openssl/ssl.h

kaduk · 2017-10-10T18:53:24Z

include/openssl/ssl.h

Need an typedef on callback type to fully fix it ; in a next PR...

kaduk · 2017-10-10T18:53:28Z

include/openssl/ssl.h

Column 92(!)

FdaSilvaYY · 2017-10-13T00:13:50Z

I was just trying to push attention on the various nits in this file.
Can you push this PR as it is ?
Else I will fix (or continue it) in a few weeks, as I'm currently far far away ... ;)

kaduk · 2017-10-16T14:47:43Z

I couldn't push anything without an OMC code review. And this seems sufficiently non-urgent that continuing when you get back should be okay.

richsalz

Do we need docs for the new typedefs?
Are the "convert to function typedefs" deliberately only partly done?

richsalz · 2017-10-16T14:52:00Z

include/openssl/ssl.h

I would remove the space.

richsalz · 2017-10-16T14:52:37Z

include/openssl/ssl.h

and remove the spaces between the function name and the param lists here, and below, as well.

Too much too fix ;)

richsalz · 2017-10-16T14:53:06Z

include/openssl/ssl.h

all those function typedefs, and there isn't one here? :)

I start these kind of changes on my git repo, as draft for a next PR.

FdaSilvaYY · 2018-01-27T14:49:21Z

Fixed and rebased to current master.
Ping @openssl for review ; it is only style nits, no code change.

kaduk · 2018-03-12T21:14:59Z

I'm not sure how much sense it makes to take a style change that is making changes to idioms not yet codified in the style guide (e.g., no space between return-type parens and function-arg parens for function typedefs). So maybe this should move to post-1.1.1?

kaduk · 2018-03-12T21:54:27Z

I'm not sure how much sense it makes to take a style change that is making changes to idioms not yet codified in the style guide (e.g., no space between return-type parens and function-arg parens for function typedefs). So maybe this should move to post-1.1.1?

Okay, that is extremely funny given that I am the one who asked for the change in the first place. I guess I should look at the rest of the changes now

kaduk

Just one nit left that I see.

kaduk · 2018-03-12T21:58:31Z

include/openssl/ssl.h

+                                          int op, int bits, int nid,
+                                          void *other, void *ex));
+int (*SSL_get_security_callback(const SSL *s)) (const SSL *s, const SSL_CTX *
+                                                ctx, int op, int bits, int nid,


The ctx and its * should be adjacent, with no space or newline separating them.

kaduk · 2018-03-12T22:07:35Z

include/openssl/ssl.h

 const CTLOG_STORE *SSL_CTX_get0_ctlog_store(const SSL_CTX *ctx);

-# endif /* OPENSSL_NO_CT */
+# endif                         /* OPENSSL_NO_CT */


This seems to be the only comment on #endif in the file; I almost wonder if it is "more consistent" with the current style to remove it than to reindent it.

This change comes from the openssl-format-source tool. I don't know the reason/rule behind it.

@kaduk: I will be glad to remove it, if you say so.
On the other side, I'm for having this kind of comment when the length of this block exceeds the height of the screen ;)
I rebase these changes on a fresh master .

I think the comment should remain. I don't know why the tool reindented it. I think it looks better without the reindentation.

Fixed and rebased to last master.

mattcaswell · 2018-03-21T19:24:18Z

@kaduk any more comments?

kaduk · 2018-03-21T23:25:26Z

I probably won't get a chance to look again this week; let me dismiss my review at least for now.

Additional changes intended to address my comments were made.

FdaSilvaYY · 2018-04-02T12:29:07Z

@kaduk any more comments, or this PR is good to go with its two approvals ?

kaduk

@kaduk any more comments, or this PR is good to go with its two approvals ?

Yes, but we should feel free to treat them as non-blocking.

kaduk · 2018-04-09T17:02:45Z

include/openssl/ssl.h

We can nestle the *ctx with no space between

kaduk · 2018-04-09T17:07:16Z

include/openssl/ssl.h

I forget if the tooling complains if we nestle these together as int (*cb)(const

I don't remember, I keep the original style; will be fixed later with a few typedef's function-pointer .

fix some indents, and restrict to 80 cols some lines.

kaduk · 2018-04-09T18:24:12Z

(reconfirm +1)

FdaSilvaYY · 2018-04-11T12:58:12Z

Ping @richsalz , @mattcaswell : little nudge for reconfirming and merge ;)

richsalz · 2018-04-11T14:16:50Z

@mattcaswell you want to reconfirm, and possibly merge?

FdaSilvaYY · 2018-04-17T20:56:00Z

Ping @richsalz , @mattcaswell this looks like it is lost in the limbo ? ;)

richsalz · 2018-04-17T21:20:19Z

@mattcaswell or @kaduk should merge (I try to avoid overlap with co-workers).

mattcaswell · 2018-04-18T08:06:24Z

Sorry for the delay. Pushed now. Thanks.

fix some indents, and restrict to 80 cols some lines. Reviewed-by: Ben Kaduk <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #4466)

FdaSilvaYY force-pushed the ssl_h_header_fix branch from 93a822a to 6c95224 Compare October 4, 2017 23:03

kaduk suggested changes Oct 10, 2017

View reviewed changes

richsalz suggested changes Oct 16, 2017

View reviewed changes

FdaSilvaYY force-pushed the ssl_h_header_fix branch 3 times, most recently from 1d86007 to c9a3354 Compare November 5, 2017 20:53

FdaSilvaYY force-pushed the ssl_h_header_fix branch from c9a3354 to 365279e Compare November 13, 2017 23:06

FdaSilvaYY force-pushed the ssl_h_header_fix branch 2 times, most recently from d76a38b to bf7dc77 Compare November 26, 2017 19:48

FdaSilvaYY force-pushed the ssl_h_header_fix branch 2 times, most recently from 30263a8 to 3ab0412 Compare December 21, 2017 20:41

FdaSilvaYY force-pushed the ssl_h_header_fix branch from 3ab0412 to f63280a Compare January 10, 2018 21:28

FdaSilvaYY force-pushed the ssl_h_header_fix branch from f63280a to fe21872 Compare January 19, 2018 19:58

mattcaswell added this to the 1.1.1 milestone Jan 24, 2018

FdaSilvaYY force-pushed the ssl_h_header_fix branch from fe21872 to 8038ed3 Compare January 27, 2018 14:42

richsalz approved these changes Jan 27, 2018

View reviewed changes

kaduk previously requested changes Mar 12, 2018

View reviewed changes

FdaSilvaYY force-pushed the ssl_h_header_fix branch 2 times, most recently from 796b21f to e3ea549 Compare March 14, 2018 18:15

FdaSilvaYY force-pushed the ssl_h_header_fix branch from e3ea549 to 1002a0a Compare March 21, 2018 18:18

mattcaswell approved these changes Mar 21, 2018

View reviewed changes

FdaSilvaYY force-pushed the ssl_h_header_fix branch from 1002a0a to 18ab167 Compare April 8, 2018 21:51

kaduk approved these changes Apr 9, 2018

View reviewed changes

FdaSilvaYY added 2 commits April 9, 2018 19:46

Style: ssl.h

8bfa56a

fix some indents, and restrict to 80 cols some lines.

squash! review comments

6fb13c9

FdaSilvaYY force-pushed the ssl_h_header_fix branch from 18ab167 to 6fb13c9 Compare April 9, 2018 17:49

mattcaswell approved these changes Apr 18, 2018

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Apr 18, 2018

mattcaswell closed this Apr 18, 2018

levitte pushed a commit that referenced this pull request Apr 18, 2018

Style: ssl.h

2f8271e

fix some indents, and restrict to 80 cols some lines. Reviewed-by: Ben Kaduk <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #4466)

FdaSilvaYY deleted the ssl_h_header_fix branch April 28, 2018 22:22

Uh oh!

Comments

Conversation

FdaSilvaYY commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaduk commented Oct 16, 2017

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented Jan 27, 2018

Uh oh!

kaduk commented Mar 12, 2018

Uh oh!

kaduk commented Mar 12, 2018

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

kaduk commented Mar 21, 2018

Uh oh!

FdaSilvaYY commented Apr 2, 2018

FdaSilvaYY commented Oct 4, 2017 •

edited

Loading

FdaSilvaYY commented Oct 13, 2017 •

edited

Loading

FdaSilvaYY Oct 31, 2017 •

edited

Loading

FdaSilvaYY Mar 13, 2018 •

edited

Loading