-
Notifications
You must be signed in to change notification settings - Fork 1k
Add C++ module support #1288
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
base: master
Are you sure you want to change the base?
Add C++ module support #1288
Conversation
|
@COM8 Do you know anything about the failing tests? I haven't touched anything relating to those why is why I'm confused about it, nothing I touched should have any effect on the tests |
COM8
left a comment
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.
@mikomikotaishi thanks for contributing! Yes this is a welcome suggestion.
Looks like clang-tidy, cmake and cppcheck updated. Those are not your issues there. I will create a MR in the next couple of days to deal with them.
I have a couple of requests:
- Please change the
cmake_minimum_versiontocmake_minimum_required(VERSION 3.15...4.1)so we can enable more default policies if available. - Should we add some integration test to ensure it works?
- Can we already use
import std;? - You are implementing the module support "the clang way" by taking care of
using. I personally prefer the MSVC way of doing it with adding a macro in front of everything (https://youtu.be/7WK42YSfE9s?si=ulBcp6Y2BA9PzN9r&t=2247). For me this has the advantage that we do not have to maintain this separate file with all includes and instead we keep changes close to our code. - I personally prefer the file extension
.cxxsince it is only 3 characters and to me if is the logical successor of.hppand.cpp. - Do we need
set(CMAKE_CXX_SCAN_FOR_MODULES ON)? - We need a CI run for this to test the build.
- Are there other modules we can consume e.g. gtest and curl?
- Why do we need the explicit inline (https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-inline-specifier.html)?
6ceb7b8 to
6676886
Compare
|
To answer your questions:
Everything else, I can make the changes now. |
6676886 to
5e0e1b9
Compare
|
@COM8 Hi, I'm sorry for the long delay. I've made a CI for building the module, but I don't imagine there is any need to make one to "test" the features of the modules, as it is just a re-export of the library. |
This PR adds support for C++20 modules, using option
CPR_BUILD_MODULES, which builds a modulecpr.