-
Notifications
You must be signed in to change notification settings - Fork 122
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 C++98 compatibility in our header files #2193
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2193 +/- ##
=======================================
Coverage 79.04% 79.04%
=======================================
Files 612 612
Lines 106222 106222
Branches 15008 15006 -2
=======================================
+ Hits 83959 83965 +6
+ Misses 21612 21606 -6
Partials 651 651 ☔ View full report in Codecov by Sentry. |
Pre-push check is failing:
|
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.
Can you update c99_gcc_test.sh to also check for C++ header compatibility (and maybe rename the file). Also double check that it's running, I'm not sure anything defines AWSLC_C99_TEST right now....
82c3577
to
cb654a3
Compare
cb654a3
to
b98b993
Compare
@@ -24,8 +24,7 @@ INCLUDE_FILES=`ls $INCLUDE_DIR/openssl/*.h | grep -v $INCLUDE_DIR/openssl/arm_ar | |||
# some non-ISO practices, but not all — only those for which ISO C requires a | |||
# diagnostic, and some others for which diagnostics have been added." | |||
# https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html | |||
|
|||
${CC} -std=c99 -c -I${INCLUDE_DIR} -include ${INCLUDE_FILES} -Wpedantic -fsyntax-only -Werror | |||
${CC} -std=c99 -c -I${INCLUDE_DIR} $(echo ${INCLUDE_FILES} | sed 's/[^ ]* */-include &/g') -Wpedantic -fsyntax-only -Werror ./tests/compiler_features_tests/builtin_swap_check.c |
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.
What is this sed removing from the list? And why add the builtin_swap_check to all of these targets?
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.
sed's mainly in charge of prepending an -include
to each file in $INCLUDE_FILES
. What we had previously didn't actually work :(. This also needs a basic "c" file to compile, so I chose builtin_swap_check
since that was a good existing example that works with older compilers
Issues:
Addresses
CryptoAlg-2693
Description of changes:
We encountered this when trying to compile Xtrabackup on gcc versions lower than 10. Apparently scoped enums are a C++11 feature, which the compiler can't understand without additional linker flags.
We also encountered this in 42a8dba, which prompted us to move it to
ssl.h
. Upon closer examination, I don't think the C++11 usage is a necessity especially with the pure C fallback that was included. Predeclaring enums reduces compilation dependencies and build times, but our build seems to work fine without it. Removing this would let us encounter less build issues with projects that aren't expecting C++ in our header files.Call-outs:
N/A
Testing:
CI
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.