Skip to content

Undefine MBEDTLS_MPI_MAX_SIZE #220

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

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Dec 4, 2018

Undefine MBEDTLS_MPI_MAX_SIZE in case it is already defined
in the configuration file.
This should avoid compilation warnings and unintended behaviour, once ARMmbed/mbed-os#8936 will be merged

@k-stachowiak
Copy link
Contributor

Just a minor note: merging ARMmbed/mbed-os#8936 will not actually cause trouble until the next Mbed TLS update in the Mbed OS.

@RonEld
Copy link
Contributor Author

RonEld commented Dec 4, 2018

will not actually cause trouble until the next Mbed TLS update in the Mbed OS.

Correct, I explained it wrong

@RonEld RonEld requested a review from AndrzejKurek December 4, 2018 09:47
@@ -33,6 +33,9 @@
* Set this value higher to enable handling larger keys, but be aware that this
* will increase the stack usage.
*/
#if defined(MBEDTLS_MPI_MAX_SIZE)
#undef MBEDTLS_MPI_MAX_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to unconditionally undef this. There are no warnings if it isn't defined, right?

Undefine `MBEDTLS_MPI_MAX_SIZE` in case it is already defined
in the configuration file.
@RonEld RonEld force-pushed the undefine_mpi_max_size branch from 8b17022 to 0b991cb Compare December 4, 2018 11:42
@RonEld
Copy link
Contributor Author

RonEld commented Dec 4, 2018

@Patater I changed according to your comment, in the same commit, as the change is minimal

@RonEld
Copy link
Contributor Author

RonEld commented Dec 4, 2018

cc @k-stachowiak

Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

I liked the ifdeffed version a bit more, but it is still correct and, apparently, popular, so LGTM

@simonbutcher
Copy link
Contributor

I can understand this PR is required within the constraints of the current configuration system, but I think #under'ing constants in the example is a maintenance issue and brittle to changes in the main configuration file. We really need to fix our config system.

@simonbutcher
Copy link
Contributor

retest

@simonbutcher simonbutcher merged commit dc50b7c into ARMmbed:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants