Skip to content

Conversation

@mikomikotaishi
Copy link

This PR adds support for C++20 modules, using option CPR_BUILD_MODULES, which builds a module cpr.

@mikomikotaishi
Copy link
Author

@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

Copy link
Member

@COM8 COM8 left a 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:

  1. Please change the cmake_minimum_version to cmake_minimum_required(VERSION 3.15...4.1) so we can enable more default policies if available.
  2. Should we add some integration test to ensure it works?
  3. Can we already use import std;?
  4. 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.
  5. I personally prefer the file extension .cxx since it is only 3 characters and to me if is the logical successor of .hpp and .cpp.
  6. Do we need set(CMAKE_CXX_SCAN_FOR_MODULES ON)?
  7. We need a CI run for this to test the build.
  8. Are there other modules we can consume e.g. gtest and curl?
  9. Why do we need the explicit inline (https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-inline-specifier.html)?

@mikomikotaishi
Copy link
Author

To answer your questions:

  • We could certainly add CI tests, I have seen them before, but I don't know much about it before.
  • import std; doesn't work in CMake in my experience. It's hidden behind an experimental flag which requires a UUID or some string that has to be found online, and changes each update, if I recall correctly. I think it's possible if you manually compile it and link it, which is plausible for small projects (a single file that imports std, for example)
  • It seems like a very herculean task to migrate the whole project into the MSVC-style, but I'm not against it. Could we do it in a subsequent PR, perhaps?
  • I haven't used set(CMAKE_CXX_SCAN_FOR_MODULES ON) before, so I don't think we need it to my knowledge, but if you know more please tell me.
  • I don't know if gtest has a module, and if I am understanding your question libcurl isn't modularisable due to being C. Please do clarify if I am not understanding correctly.
  • I assume you are asking about <cpr/status_codes.h>, these constants have to be inline because they need external linkage to be exported from a module.

Everything else, I can make the changes now.

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.

2 participants