Typo and style by FdaSilvaYY · Pull Request #4457 · openssl/openssl

FdaSilvaYY · 2017-10-03T20:46:43Z

Few fixes staging on my repo

Checklist

documentation is added or updated
tests are added or updated

levitte · 2017-10-04T08:44:16Z

doc/man3/SSL_read_early_data.pod

an efficient

kaduk · 2017-10-04T13:00:19Z

doc/man3/SSL_read_early_data.pod

This one seems like a matter of taste, but I don't really prefer one over the other, myself.

'performant' is missing in various english dictionaries...

kaduk · 2017-10-04T13:01:26Z

include/openssl/ssl.h

Huh, the old form is surprising. I could understand breaking early to get 'data' and 'len' on the same line, but the old form seems to have not had any real benefit.

kaduk · 2017-10-04T13:01:59Z

include/openssl/ssl.h

Please revert this chunk; SSL *s is the prevailing style here.

OK, done.
I think (and wish) that one letter should be forbidden for non-POD type.
It 's a pain to track them in the code .
May be a future addition to Style Guideline ? ;)

kaduk · 2017-10-04T13:04:46Z

include/openssl/ssl.h

For these sort of callback getters, I'd almost consider a more radical solution that indents the getter arguments only a few columns more than, say, the start of the getter function name. But definitely wait for a second opinion before implementing that!

A better solution struct ssl_st* -> SSL* ...
plus a typedef on callback functions ...

I did not dig the origins of this kind of declarations ...

kaduk · 2017-10-04T13:05:41Z

include/openssl/ssl.h

Looks like the __owur addition was a little sloppy.

kaduk · 2017-10-04T13:42:16Z

include/openssl/ssl.h

I would prefer to avoid splitting the type and name across lines like this.
Similar comments as to callback getters (above) apply, and I suppose we also retain the option of not specifying a name at all.

kaduk · 2017-10-04T13:48:31Z

include/openssl/ssl.h

Is this going from column 79 to column 83?!

Not on my side.

kaduk · 2017-10-04T13:49:35Z

include/openssl/ssl.h

It looks like we prefer to keep the )( nestled together for the function (type) arguments.
Also, no space in *ctx.

kaduk · 2017-10-04T13:52:37Z

include/openssl/ssl.h

Huh, this is the only instance in ssl.h of endif-with-comment.
I guess your change is consistent with ssl_locl.h, so I don't really have grounds to object.

kaduk · 2017-10-04T13:53:35Z

include/openssl/ssl.h

Isn't this trailing comma still column 81?

yes it is ... jut one out, not the worst case of this file ;)

FdaSilvaYY · 2017-10-04T18:40:44Z

Should I split out ssl.h corrections into his standalone PR to ease the Approval ?

kaduk

Okay, the non-ssl.h changes look fine.

FdaSilvaYY · 2017-10-31T22:09:37Z

Rebased to latest master

FdaSilvaYY · 2017-11-07T15:43:30Z

Comments fixed, and PR rebased to last master.

levitte · 2017-11-07T15:55:35Z

ssl/record/rec_layer_s3.c

nit pick. The comment needs to move one space to the right

kaduk · 2017-11-07T18:08:02Z

reconfirm +1
I'll merge.

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #4457)

kaduk · 2017-11-07T18:18:18Z

Merged to master; closing

FdaSilvaYY force-pushed the typo_n_style branch 3 times, most recently from cf094e1 to 3fa8867 Compare October 3, 2017 21:49

levitte requested changes Oct 4, 2017

View reviewed changes

doc/man3/SSL_read_early_data.pod Outdated

Copy link

Member

levitte Oct 4, 2017

Choose a reason for hiding this comment

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

an efficient

FdaSilvaYY force-pushed the typo_n_style branch from 3fa8867 to d17e19d Compare October 4, 2017 09:10

kaduk suggested changes Oct 4, 2017

View reviewed changes

FdaSilvaYY force-pushed the typo_n_style branch from d17e19d to e0d91bc Compare October 4, 2017 18:34

FdaSilvaYY force-pushed the typo_n_style branch from e0d91bc to ae7e2a6 Compare October 4, 2017 22:58

FdaSilvaYY mentioned this pull request Oct 4, 2017

Style: ssl.h #4466

Closed

1 task

kaduk approved these changes Oct 6, 2017

View reviewed changes

FdaSilvaYY force-pushed the typo_n_style branch from ae7e2a6 to e860bde Compare October 31, 2017 22:09

FdaSilvaYY force-pushed the typo_n_style branch from e860bde to da905f1 Compare November 7, 2017 15:40

levitte requested changes Nov 7, 2017

View reviewed changes

FdaSilvaYY added 2 commits November 7, 2017 17:09

style : fix some if(...

2cd810a

Various typo

0142c00

FdaSilvaYY force-pushed the typo_n_style branch from da905f1 to 0142c00 Compare November 7, 2017 16:09

levitte approved these changes Nov 7, 2017

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Nov 7, 2017

levitte pushed a commit that referenced this pull request Nov 7, 2017

style : fix some if(...

f479eab

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #4457)

levitte pushed a commit that referenced this pull request Nov 7, 2017

Various typo

f4411fa

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #4457)

kaduk closed this Nov 7, 2017

FdaSilvaYY deleted the typo_n_style branch November 7, 2017 20:37

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Oct 4, 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!

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 Oct 4, 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 Oct 4, 2017

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented Oct 31, 2017

Uh oh!

FdaSilvaYY commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk commented Nov 7, 2017

Uh oh!

kaduk commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FdaSilvaYY commented Oct 3, 2017 •

edited

Loading

FdaSilvaYY Oct 4, 2017 •

edited

Loading

FdaSilvaYY Oct 4, 2017 •

edited

Loading