-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix nonstandard C++ in headers #7194
base: development
Are you sure you want to change the base?
Fix nonstandard C++ in headers #7194
Conversation
The PAKE structure initialization uses a named initializer, which is not supported in versions of C++ that are still widespread. The named initializer doesn't really help here, so get rid of it. More generally, this commit simplifies the initialization of the structure by always including the dummy field. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
d5fb1a1
to
7b4eff9
Compare
This avoids us accidentally using language features in headers that are not present in standard C++ or were added very recently. I used C++11 as the cutoff because our code base relies on several features that aren't present in C++98, including commas at the end of enumerator lists which have been present in the Mbed TLS code base since the PolarSSL days. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
#6567 appears to have made similar changes. Does that obsolete/supersede this? |
@RytoEX Thanks for making the link! #6567 obsoletes the PAKE context changes, but the addition of C++ pedantic application builds in this PR are still relevant. I'll rebase. Also, since #6557 fixed part of a known issue, I guess we should write a changelog entry. Or maybe we should wait for a full fix? |
Good point.
My understanding is that #7087 is composed of multiple issues/goals. It might be helpful to have a changelog entry specifically about resolving the Designated Initializers issue so that it is more easily found (I had to dig into git blame), while leaving #7087 open until all of its goals are achieved (if its scope is to remain as-is). That leaves:
That said, I leave whether to write a changelog entry now to the discretion of you and your colleagues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This needs rebasing as pointed out previously. Regarding the ChangeLog, I think we should have a bugfix entry here about the compilation failure in C++ mode (even though it was fixed in #6567 without noticing). Regarding backport, I don't think the issue is present in 2.28, but the testing improvement should be backported of course. |
Fix some code in header files that is standard C99, but not standard C++11.
I use C++11 as the cutoff because Mbed TLS headers have never been pedantically compliant C++98 due to the use of commas at the end of enumerator lists (and perhaps other features).
This is work in progress addressing #7087. It fixes the initializers, but not the problem with linkage of functions that return structs.
Gatekeeper checklist