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.
@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. |
|
@mikomikotaishi thanks for making the requested changes. If you take care of rebasing, I can take care of migrating it to the MSVC way of doing it. |
f7b110a to
0f461ce
Compare
|
OK, I've rebased. |
This PR adds support for C++20 modules, using option
CPR_BUILD_MODULES, which builds a modulecpr.