Code improvement #379
Code improvement #379
Conversation
benquike
commented
Feb 28, 2021
benquike
commented
- Remove unneeded Makefiles
- Factor out llvm_libs setup
- cleanup CXX_FLAGS setup
CMakeLists.txt
Outdated
| # add -std=gnu++14 | ||
| set(CMAKE_CXX_EXTENSIONS ON) | ||
|
|
||
| add_compile_definitions(HAVE_IEEE_754) |
There was a problem hiding this comment.
Thanks for your contribution:) Could this be moved to the CMakeList under CUDD since these marcos are only used there?
There was a problem hiding this comment.
Updated, please have a look.
| else() | ||
| set(CMAKE_CXX_FLAGS "-fPIC -std=gnu++14 -O3 -fno-rtti -Wno-deprecated -DHAVE_IEEE_754 -DSIZEOF_VOID_P=8 -DSIZEOF_LONG=8") | ||
| endif() | ||
| set(CMAKE_C_FLAGS "-fPIC") |
There was a problem hiding this comment.
Can we still keep -fPIC or did I miss anything in your patch?
There was a problem hiding this comment.
PIC is enabled by default on recent Linux distributions, to support ASLR.
So it is duplicated.
There was a problem hiding this comment.
Just in case, someone is built on an old version of Linux.
There was a problem hiding this comment.
Yes. Update.
Please have a look.
Also, I have removed -Wno-deprecated, as the current code can be built now without it.
CMakeLists.txt
Outdated
| if(CMAKE_BUILD_TYPE MATCHES "Debug") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0") | ||
| else() | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O3") | ||
| endif() |
There was a problem hiding this comment.
would be nice to adjust these indents.
There was a problem hiding this comment.
It is ready to merge once you fix these indents. It looks not properly aligned. Thanks
| set(CMAKE_CXX_EXTENSIONS ON) | ||
|
|
||
| set(COMMON_FLAGS "-fPIC -Werror") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${COMMON_FLAGS} -fno-rtti") |
There was a problem hiding this comment.
Sorry for one more thing here. Lines 20-21 seems redundant if we have Lines 25-29. Could you merge them to make it compact?
There was a problem hiding this comment.
No. They are not redundant.
25-29 are only for setting up -Ox under different building modes, while 21-22 are for setting up options for C and CXX sources in common.
