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.

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

Add parentheses on public macros where appropriate #3100

Conversation

bernd-edlinger
Copy link
Member

Code-health, although today is not Tuesday :)
Fixes #3063.

@richsalz richsalz self-assigned this Apr 1, 2017
@richsalz richsalz added branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch 1.1.0 branch: master Merge to master branch labels Apr 1, 2017
@bernd-edlinger
Copy link
Member Author

I don't know how much of this PR can be cherry-picked for 1.1.0 and 1.0.2
but I can create extra PRs if that is considered useful for back-porting.

@richsalz
Copy link
Contributor

richsalz commented Apr 1, 2017

please do that.

@dot-asm
Copy link
Contributor

dot-asm commented Apr 1, 2017

Hmm... I for one would vote against taking it to released branches.

@richsalz
Copy link
Contributor

richsalz commented Apr 1, 2017

okay, why?

@dot-asm
Copy link
Contributor

dot-asm commented Apr 1, 2017

Because these are not bugs.

@kroeckx
Copy link
Member

kroeckx commented Apr 1, 2017

I have to agree that this should go to the stable branches.

@levitte
Copy link
Member

levitte commented Apr 1, 2017

What would the motivation be to take this to the stable branches? @dot-asm is right, these changes aren't bug fixes per se, and they don't make future backports easier (unless we're touching these specific macros, but that happens very seldom), so yeah, I also have a hard time motivating a backport...

@richsalz
Copy link
Contributor

richsalz commented Apr 1, 2017

They seem to me to fix latent bugs, such as if you do p + 1 to the BIO set-host macro.
Yes, not all of them are.

@dot-asm
Copy link
Contributor

dot-asm commented Apr 2, 2017

They seem to me to fix latent bugs,

Not quite, they rather prevent bugs. While definition of branch in maintenance mode is [actual] bug fixing. Well, we all know that we are quite liberal about this in 1.1.0 context, and this might be viewed as kind of a grey area, but public headers are are not as grey as rest of the code. Yes, there is contradiction here, public headers need it most, but their modification is not to be taken lightly.

@bernd-edlinger
Copy link
Member Author

Ping?

@mattcaswell
Copy link
Member

Approved for master only

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch 1.1.0 labels Apr 27, 2017
levitte pushed a commit that referenced this pull request Apr 27, 2017
Fixes #3063.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #3100)
@mattcaswell
Copy link
Member

Pushed. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants