Skip to content

Conversation

@emmenlau
Copy link
Contributor

The current cmake implementation uses old-style cmake syntax with global variables to set C++11 and Position Independent Code properties. This PR changes the syntax to modern cmake style.

Is this good?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ec9acf5 on BioDataAnalysis:emmenlau_use_modern_cmake_for_cpp11 into ffc1a3e on SRombauts:master.

@SRombauts SRombauts self-assigned this Jan 17, 2020
@SRombauts
Copy link
Owner

I am unsure of the benefit of this.

What do you think of this other PR #239 that will be incompatible with this?
Basically, we should enforce a minimum version, not an explicit version.
So if people want to use C++17 it's fine, while right now I may have enforced C++11 for everyone (losing some feature we have in the wrapper)

@emmenlau
Copy link
Contributor Author

emmenlau commented Jan 17, 2020

I fully understand. But it may help to split the "question" into two parts:

I do recommend to use modern cmake syntax wherever possible. This is because it brings often underlying benefits, like transitivity of properties, and easier maintenance of the code in the future. For example, setting position-independent code can be passed down via cmake flags to consumers of this library. My PR does not (yet) include this change. But it makes better use of modern cmake syntax than the current code. So my PR is only about the syntax.

I do not mean to suggest that a higher c++ standard should be enforced. I agree that this should be up to the user of the library. While I think that c++11 is fine now, I still agree that more modern features may not bring a big benefit to the larger community but may exclude many users. My personal recommendation would be to make cmake options for specific features that require a higher c++ standard. For example add something like:

option(ENABLE_XXX "Allow to use feature XXX. Requires c++14." OFF)
if(ENABLE_XXX)
    target_compile_definitions(${PROJECT_NAME} PUBLIC SQLITECPP_ENABLE_XXX)
    set(SQLITECPP_MINIMUM_CXX_STANDARD 14)
endif()
[...]

set_target_properties(${PROJECT_NAME} PROPERTIES CXX_STANDARD ${SQLITECPP_MINIMUM_CXX_STANDARD})

Users will be able to choose for the features they require, and you can deduce the required c++ standard based on that. Personally I think that is the gold standard you can achieve. But it makes your code sightly more complex because you need to separate out modern features based on the additional defines.

What do you think?

@emmenlau
Copy link
Contributor Author

Oh and by the way, I think PR #239 and my change here are perfectly compatible. Only the syntax of #239 would need to be slightly changed.

@emmenlau
Copy link
Contributor Author

Closing this in favor of #269

@emmenlau emmenlau closed this Jan 30, 2020
@emmenlau emmenlau deleted the emmenlau_use_modern_cmake_for_cpp11 branch January 30, 2020 13:32
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.

3 participants