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

Renaming "new" variable #1783

Merged
merged 5 commits into from
Jun 29, 2018
Merged

Renaming "new" variable #1783

merged 5 commits into from
Jun 29, 2018

Conversation

hirotakaster
Copy link

@hirotakaster hirotakaster commented Jun 25, 2018 •
edited by RonEld
Loading

Description

Based on issue #1782
Replace *new to *new_cert in ssl_append_key_cert function at ssl_tls.c.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

@hanno-becker hanno-becker self-requested a review June 25, 2018 10:24
@hanno-becker hanno-becker self-assigned this Jun 25, 2018
hanno-becker
hanno-becker suggested changes Jun 25, 2018
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

The actual change looks good to me. Could you please add an entry in the ChangeLog, including the platform on which the previous naming causes the build failure, the way you want to be credited, and the issue that this PR fixes? Thank you!

@RonEld
Copy link
Contributor

RonEld commented Jun 25, 2018 •
edited
Loading

@hirotakaster Thank you for your contribution!

In addition, unfortunately our policy is to not accept contributions, without a Contributor’s Licence Agreement (CLA) signed or authorised by yourself or your employer.
If this is a personal contribution, the easiest way to do this is if you create an mbed account and accept this click through agreement. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.
Thanks for your understanding and again, thanks for the contribution!

@hirotakaster
Copy link
Author

@RonEld
I accepted CLA now.
thank you for your kindly.

@RonEld
Copy link
Contributor

RonEld commented Jun 25, 2018 •
edited
Loading

@hirotakaster Thank you for accepting the CLA! Could you kindly confirm that this is your Mbed account?

@hirotakaster
Copy link
Author

@RonEld
Yes, this is my mbed account.

RonEld
RonEld suggested changes Jun 25, 2018
View reviewed changes
ChangeLog Outdated

Bugfix
* Fix Renaming "new" variable #1783.
compile error(new variable) with arm-none-eabi-gcc(c++) on mbed TLS 2.7.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this sentence to:

Fix compilation error on c++, because of a variable named new. Found and fixed by Hirotaka Niisato in #1783

Unless you want to credit yourself with your github name.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I'm okay "Hirotaka Niisato" .

@hirotakaster
Copy link
Author

@hanno-arm
I add ChangeLog to this PR now.
Thank you for your kindness.

@hirotakaster
Copy link
Author

Thank you @RonEld, I updated ChangeLog now according to your advice.

@RonEld RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 25, 2018
hanno-becker
hanno-becker suggested changes Jun 25, 2018
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Minor change in the ChangeLog entry requested.

ChangeLog Outdated
= mbed TLS x.x.x branch released xxxx-xx-xx

Bugfix
* Fix compilation error on c++, because of a variable named new. Found and fixed by Hirotaka Niisato in #1783
Copy link

Choose a reason for hiding this comment

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

Please split this into multiple lines to avoid lines of more than 80 characters. Also, please add a full stop at the end of the second sentence, and capitalize C++.

Copy link
Author

Choose a reason for hiding this comment

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

@hanno-arm
Now I update the ChangeLog.
Thank you.

hanno-becker
hanno-becker approved these changes Jun 25, 2018
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @hirotakaster for the quick turnaround!

@hanno-becker hanno-becker removed the needs-review Every commit must be reviewed by at least two team members, label Jun 25, 2018
@hanno-becker
Copy link

@sbutcher-arm Ready to be merged from our perspective.

RonEld
RonEld approved these changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

Approving also latest changes to ChangeLog

@simonbutcher simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Jun 27, 2018
@simonbutcher simonbutcher added the needs-backports Backports are missing or are pending review and approval. label Jun 27, 2018
@simonbutcher
Copy link
Contributor

Needs backporting to mbedtls-2.7, although I think that can be done with this PR without problem.

@simonbutcher simonbutcher merged commit 164b9cd into Mbed-TLS:development Jun 29, 2018
This was referenced Jun 29, 2018
Merged
Merged
@simonbutcher
Copy link
Contributor

Backported to mbedtls-2.7 with PR #1819 and to mbedtls-2.1 with PR #1820.

@simonbutcher simonbutcher removed the needs-backports Backports are missing or are pending review and approval. label Jun 29, 2018
@simonbutcher simonbutcher mentioned this pull request Jul 11, 2018
Closed
@hirotakaster hirotakaster mentioned this pull request Jan 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanno-becker hanno-becker hanno-becker approved these changes

@RonEld RonEld RonEld approved these changes

Assignees

@hanno-becker hanno-becker

Labels
approved Design and code approved - may be waiting for CI or backports bug component-tls
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hirotakaster @RonEld @hanno-becker @simonbutcher