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

🔧 gcc-4.8.5 support #68

Merged
merged 1 commit into from
Feb 13, 2022
Merged

🔧 gcc-4.8.5 support #68

merged 1 commit into from
Feb 13, 2022

Conversation

fargies
Copy link

@fargies fargies commented Feb 11, 2022

  • Add polyfills for missing stl templates
  • some move related type_traits are not available as builtins, support is dropped
  • disable broken -Wshadow
  • disable sanitize and benchmark features when old compiler is detected (both require recent compilers)

Note: no modifications will be required on cmake repo, it felt better to add a repo specific CMake file here, and modification on gtest were adding flags in global scope anyway (the -include are not needed to compile c4core itself).

I ran tests on Github (coverage fails du to missing creds) and ensure header was present in amalgamated version.

Copy link
Owner

@biojppm biojppm left a comment

Choose a reason for hiding this comment

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

@fargies thanks for the PR. It's great; there's just some minor points to be addressed. In particular, adding the Debug tests in the CI is important because those are a lot more thorough than the Release.

.github/workflows/test.yml Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
src/c4/charconv.hpp Show resolved Hide resolved
compat.cmake Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #68 (aaf2d67) into master (79c15b0) will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   93.96%   94.10%   +0.14%     
==========================================
  Files          53       53              
  Lines       11427    11381      -46     
==========================================
- Hits        10737    10710      -27     
+ Misses        690      671      -19     
Impacted Files Coverage Δ
src/c4/charconv.hpp 97.55% <ø> (-0.04%) ⬇️
test/test_allocator.cpp 100.00% <ø> (ø)
test/test_error.cpp 88.35% <ø> (ø)
src/c4/ext/fast_float_all.h 26.81% <0.00%> (-1.31%) ⬇️
test/test_memory_resource.cpp 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 255cb55...aaf2d67. Read the comment docs.

@biojppm
Copy link
Owner

biojppm commented Feb 11, 2022

@fargies the coverage test failures seem to be spurious. However, the single header test failures (eg, here) seem to be meaningful. Can you investigate? Something like this (give or take) should be enough to figure out what may be going on.

$ cd test/test_singleheader/
$ cmake -S . -B build -D C4CORE_DEV=ON
$ cmake --build build --target c4core-test-run

@fargies
Copy link
Author

fargies commented Feb 12, 2022

Not sure on how to fix that one (yet).

  • The amalgamate script keeps only first includes encountered
  • The gcc-4.8 header includes cstdint and type_traits but has conditionals around those inclusions
  • It must be included early to properly provide the replacement templates

I can see two different paths to fix this, but would require your advise:

  1. move the includes outside conditionals
  2. add a flag/comment for amalgamate script to ignore those includes (adding this feature to the script)

The script seems too basic to make a code analysis and detect the conditionals (but I may be wrong).

Which direction do you prefer, can you see another way ?

@biojppm
Copy link
Owner

biojppm commented Feb 12, 2022

Right. Yes, the removal of includes in the amalgamate facilities is too clever by half, and can't handle conditional includes.

I think the best way to proceed is to work around the problem by injecting an explicit include block. I would push to the PR myself, but am in a rush and can't do it.

Try something like this, should get you in the way:

modified   tools/amalgamate.py
@@ -29,6 +29,11 @@ def amalgamate_c4core(filename: str,
 #if defined(C4CORE_SHARED) && defined({defmacro}) && !defined(C4CORE_EXPORTS)
 #define C4CORE_EXPORTS
 #endif
+"""
+    required_gcc4_8_include = """// these includes are needed to work around conditional
+// includes in the gcc4.8 shim
+#include <cstdint>
+#include <type_traits>
 """
     srcblocks = [
         am.cmttext(f"""
@@ -51,6 +56,7 @@ INSTRUCTIONS:
 """),
         am.cmtfile("LICENSE.txt"),
         am.injcode(exports_def_code),
+        am.injcode(required_gcc4_8_include),
         "src/c4/export.hpp",
         "src/c4/preprocessor.hpp",
         "src/c4/platform.hpp",

Thanks for the patience.

@biojppm
Copy link
Owner

biojppm commented Feb 12, 2022

Oh, and be sure to remove the amalgamated header before retrying; sometimes the older one sticks around and confuses things.

@fargies
Copy link
Author

fargies commented Feb 13, 2022

Side note:

There seem to be a small side-effect to the SANITIZE option, running the commands you provided me I had to disable it otherwise ubsan/tsan/msan targets don't get the C4CORE_SINGLE_HEADER macro and fail to compile.

Adding the compile_definition at directory level fixes it (not sure you wish to have that in this PR):

--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -18,7 +18,7 @@ c4_add_library(c4core-_libtest LIBRARY_TYPE STATIC
     FOLDER test
     )
 if(C4CORE_DEFINED_FROM_SINGLEHEADER)
-    target_compile_definitions(c4core-_libtest PUBLIC -DC4CORE_SINGLE_HEADER)
+    add_compile_definitions(C4CORE_SINGLE_HEADER)
 endif()
 
 function(c4core_test name)

- Add polyfills for missing stl templates
 - disable broken -Wshadow
@fargies
Copy link
Author

fargies commented Feb 13, 2022

@biojppm PR should be ready, let me know if you want other changes

@biojppm biojppm merged commit 6db3d6b into biojppm:master Feb 13, 2022
@biojppm
Copy link
Owner

biojppm commented Feb 13, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants