Add support for C++ modules #578
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary of ChangesHello @mikomikotaishi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for C++20 modules, which is a significant and welcome improvement for modern C++ projects. The overall approach of using a primary module interface file with partitions for different serialization formats is sound. I've identified a few issues, including some critical syntax errors in the module files that will prevent compilation, a bug in the CMake configuration, and a couple of other areas for improvement regarding code correctness and maintainability. My detailed comments are below.
src/modules/rfl.cppm
Outdated
Comment on lines
304
to
345
| export namespace std { | ||
| using std::hash; | ||
| using std::swap; | ||
|
|
||
| #ifdef REFLECTCPP_USE_STD_EXPECTED | ||
| using std::bad_expected_access; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Exporting declarations into the std namespace (as done with export namespace std) results in undefined behavior according to the C++ standard. While this might work on some compilers, it is not portable and should be avoided. Consumers of your module should be responsible for bringing std symbols into scope, for example by importing the std module. Please remove this block.
Author
There was a problem hiding this comment.
I think this is necessary as otherwise the std::* specialisations won't be accessible.
Contributor
There was a problem hiding this comment.
I can't really find a source for this being UB. Maybe specializations can be exported explicitly though?
Author
There was a problem hiding this comment.
This isn't even actually "undefined behaviour", compiler vendors do this to export the std module. My guess is the AI assumed I was adding additional non-standard symbols into namespace std, but these are official symbols. I think this is necessary to export specialisations/overloads, anyway
mikomikotaishi
force-pushed
the
main
branch
2 times, most recently
from
3b0d675 to
9137f57
Compare
Contributor
|
@mikomikotaishi awesome PR. Thank you so |
Contributor
|
@mikomikotaishi , this looks great. There is one minor comment regarding your rfl module file. Also, we need automated tests for this and should update the Github Actions pipeline accordingly. Do you know how to do it or should I do it? |
Contributor
|
If I'm allowed to give some input as a user, it appears to be that the module |
Author
|
@liuzicheng1987 Did you leave a comment somewhere in the review? If so, I haven't been able to find it. @GregTheMadMonk If I am understanding correctly, the module itself is built only by including the headers but is compiled, I chose to bundle the serialisation formats into the module in full because I think this offers the simplest API for users. If you have an alternate idea, I am happy to hear it but generally want to keep |
Contributor
|
I did...I said that most of your includes in the rfl modules are redundant, because they are included in rfl.hpp anyway. |
Contributor
Author
|
Apologies, I couldn't find it. Anyway, when I was originally writing the library my IDE couldn't detect the symbols until I explicitly added those |
Contributor
|
@mikomikotaishi the problem is that with the current approach the users still have to install header dependencies for all formats instead just the ones they use. This is quite tedious, and some libraries aren't very pleasant to manage as dependencies. As a user, I'd expect that IMO a good compromise would be to have |
Author
|
That works for me, I was thinking of that. Also, @liuzicheng1987, any thoughts on prefixing the module name with the organisation (i.e. Java-style), and making the module name something like |
Contributor
|
@mikomikotaishi we shouldn't use Java style. The names of the modules should align with namespaces. |
Contributor
|
@mikomikotaishi , and by the way I agree with @GregTheMadMonk 's suggestion: There should be a way to only compile what you actually want. Some dependencies can be a bit tedious and forcing people to go through this even though it's completely unnecessary for what they actually want is a problem. So I like the solution you came up with. |
Author
|
@liuzicheng1987 This would require fine-tuned configuration, because CMake has to know ahead of time which modules need to be built. Are there already such things existing in the library? |
Contributor
|
@mikomikotaishi , yes, there are flags for that in the CMake: |
Author
|
Thanks, this is perfect. |
Author
|
I've decided that I think the best way is just to keep one module, |
Contributor
|
@mikomikotaishi, sure that works as well. |
Author
|
@liuzicheng1987 I've applied these changes now |
Contributor
|
@mikomikotaishi , awesome stuff. We're still missing the tests, though. I don't think we need to test everything, but we should make sure things compile and write some basic smoke tests. Also, I have just merged the atomic branch, which adds support for atomic variables. The three new functions in include/rfl/atomic/ should be exposed as well. Do you want do this or should I take care of it? |
Author
|
@liuzicheng1987 I've added the |
Contributor
|
@mikomikotaishi @GregTheMadMonk Not sure if modules integrate well with CMake Components |
Contributor
|
@deckstose , that's fine. The modules are just an optional add-on. We continue to support the old structure and it will remain the default for the foreseeable future. @mikomikotaishi did a very good job with his design there. |
Author
|
Hi, I'm not familiar with CMake components but I can look into it and see if there's anything that can be done about this. But until then, the simplest past forward seems to just be offering the module as representing the entire library extent chosen by the user. |
Contributor
|
@mikomikotaishi , I tweaked it a bit and tried to add tests, but it won't compile. I have pushed the current status. Do you have any ideas what is going wrong here? |
Contributor
Because otherwise I couldn't even get the library to compile. |
Author
|
@liuzicheng1987 Could you send me the error logs? Also, are you using Clang or GCC? |
Contributor
|
@mikomikotaishi ...sorry about that, it was late at night. Here are the error logs: If I build without the tests, it works fine (but only after I had removed the partition exports): But building with the tests fails: For some reason it doesn't know to add the |
Contributor
|
@liuzicheng1987 maybe GCC14 does not support the modern modules toolchain yet (they introduced the required features in either 14 or 15 and I don't remember which)? I'm on GCC15 and it is able to find the module and produces meaningful errors: ErrorsI used essentially your CMake command but substituted If your distro repos don't have GCC15, it's also worth trying Clang. P.S. Also: import std.string;
import std.vector;should be just import std;- there is only Module It's also the compilers' choice to support it in C++20 mode since strictly speaking it's a C++23-only feature. So you might also want to enable Also if your distro's |
Contributor
|
@GregTheMadMonk , this error suggests that it cannot find the rfl module and it can't even figure out it's supposed to use modules: So it's basically the same problem I have with g++ 14. Moreover, I am certain that modules work in principle, because I can easily build this simple example using g++ 14: https://stackoverflow.com/questions/57300495/how-to-use-c20-modules-with-cmake |
Contributor
|
@liuzicheng1987 nevermind my previous comment, I got so lost in the wall of text |
Contributor
|
If I replace |
Contributor
|
@GregTheMadMonk , you are right, this resolves the issue, but then the next one pops up: Apparently this is because we are importing header files into the module and |
Contributor
|
@mikomikotaishi @GregTheMadMonk , so I have bad news. Apparently we need https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p1815r2.html, which won't be implemented until g++-16. g++-16 has not been published yet. https://gcc.gnu.org/projects/cxx-status.html In other words, it is unlikely we will get this to run on g++ until module support is fully implemented. |
Contributor
|
@mikomikotaishi @GregTheMadMonk , but the good news are that it works like a charm on Clang 20 after I removed the static from rfl::default_value. Here's what I did: @GregTheMadMonk I think if you try to recompile your project with Clang, it should work. |
Contributor
|
@GregTheMadMonk , @mikomikotaishi , I just hit a bug in Clang: How should we proceed here? I mean...we got pretty far and we've done a lot of work that will be useful in the future here. However, it appears to me that C++ modules are not quite ready for prime time yet. (Not this PR...just C++ modules in general.) Maybe we should keep the branch open, but wait for a while until we merge it into main. What are your thoughts? |
Author
|
Is there any more information with the failure? Is it a compiler crash? Why not try Clang 21? Maybe it could resolve whatever the bug is here. I would support merging it into a separate branch until it's ready to be merged into main if we can't resolve the problem yet. |
Contributor
|
@mikomikotaishi , yes, it is a compiler crash. To reproduce: There is a pretty long stack trace and it appears that the issue is somehow related to the CBOR tests: |
Author
|
I remember dealing with one of these compiler crashes in an entirely different project, and it turned out to have been caused by a macro before an include changing the contents of a header. Not necessarily what is happening here, but simply what I once experienced, and it could be a similar issue. If need be I can ping the person involved with module development in this thread for more information. Do compiling the other parts of the library work, is it just failing on the rfl::cbor stuff? |
Contributor
|
Can confirm that commit On GCC15: I was able to reduce errors to only this diffdiff --git a/include/rfl/internal/find_index.hpp b/include/rfl/internal/find_index.hpp
index f32bc96..04c524e 100644
--- a/include/rfl/internal/find_index.hpp
+++ b/include/rfl/internal/find_index.hpp
@@ -36,7 +36,7 @@ constexpr auto wrap_fields(std::integer_sequence<int, _is...>) {
/// Finds the index of the field signified by _field_name
template <StringLiteral _field_name, class Fields>
-constexpr static int find_index() {
+constexpr int find_index() {
constexpr int ix = wrap_fields<_field_name, Fields>(
std::make_integer_sequence<int, rfl::tuple_size_v<Fields>>());
static_assert(rfl::tuple_element_t<ix, Fields>::name_ == _field_name,
@@ -46,7 +46,7 @@ constexpr static int find_index() {
/// Finds the index of the field signified by _field_name or -1.
template <StringLiteral _field_name, class Fields>
-constexpr static int find_index_or_minus_one() {
+constexpr int find_index_or_minus_one() {
if constexpr (rfl::tuple_size_v<Fields> == 0) {
return -1;
} else {
diff --git a/include/rfl/internal/make_tag.hpp b/include/rfl/internal/make_tag.hpp
index 6c5d12d..9d177e0 100644
--- a/include/rfl/internal/make_tag.hpp
+++ b/include/rfl/internal/make_tag.hpp
@@ -14,7 +14,7 @@
namespace rfl::internal {
template <internal::StringLiteral _discriminator, class T>
-static inline auto make_tag(const T& _t) noexcept {
+auto make_tag(const T& _t) noexcept {
if constexpr (internal::has_reflection_type_v<T>) {
return make_tag<_discriminator>(_t.reflection());
} else if constexpr (named_tuple_t<T>::Names::template contains<
diff --git a/include/rfl/parsing/is_empty.hpp b/include/rfl/parsing/is_empty.hpp
index 4c596d3..6f59703 100644
--- a/include/rfl/parsing/is_empty.hpp
+++ b/include/rfl/parsing/is_empty.hpp
@@ -14,7 +14,7 @@ namespace rfl {
namespace parsing {
template <class T>
-static bool is_empty(const T& _var) {
+bool is_empty(const T& _var) {
using Type = std::remove_cvref_t<T>;
if constexpr (std::is_pointer_v<Type>) {
return !_var || is_empty(*_var);
diff --git a/include/rfl/parsing/make_type_name.hpp b/include/rfl/parsing/make_type_name.hpp
index 5f2ed40..393878c 100644
--- a/include/rfl/parsing/make_type_name.hpp
+++ b/include/rfl/parsing/make_type_name.hpp
@@ -14,7 +14,7 @@ inline std::string replace_non_alphanumeric(std::string _str) {
}
template <class U>
-static std::string make_type_name() {
+std::string make_type_name() {
if constexpr (is_tagged_union_wrapper_v<U>) {
return replace_non_alphanumeric(type_name_t<typename U::Type>().str() +
"__tagged");
diff --git a/src/modules/CMakeLists.txt b/src/modules/CMakeLists.txt
index a29abce..0f05505 100644
--- a/src/modules/CMakeLists.txt
+++ b/src/modules/CMakeLists.txt
@@ -84,6 +84,7 @@ target_include_directories(reflectcpp_module PUBLIC
)
target_link_libraries(reflectcpp_module PUBLIC reflectcpp::reflectcpp)
+target_compile_definitions( reflectcpp_module PRIVATE yyjson_api_inline=yyjson_inline )
add_library(reflectcpp::module ALIAS reflectcpp_module)With this, I only get the following errors: Error logTBH I don't even see why those are considered TU-local... their declarations look like they'd be Note that this is only with default backends set, there are more errors with Clang 21 errors with `REFLECTCPP_ALL_FORMATS=ON`I also have a question about template <rfl::internal::StringLiteral name>
struct S { rfl::Rename<name, int> a = 10; };I feel like I have to use it for |
Contributor
|
Can't confirm compiler crashing on Clang 20 ( There are regular C++ errors, but no compiler crash: Error log |
Contributor
|
@GregTheMadMonk , hmm...these errors appear to be emanating from the jsoncons library, which we use as the underlying library for CBOR and UBJSON. However, it must have something to with modules. When compile the exact same thing, but without modules, it works just fine, no compiler crash, no errors: ubuntu@dev:~/reflect-cpp$ cmake -S . -B build -DCMAKE_CXX_STANDARD=23 -DCMAKE_BUILD_TYPE=Release -G Ninja -DCMAKE_CXX_COMPILER=clang++-20 -DREFLECTCPP_BUILD_TESTS=ON -DREFLECTCPP_MSGPACK=ON -DREFLECTCPP_BSON=ON -DREFLECTCPP_CBOR=ON -DREFLECTCPP_FLEXBUFFERS=ON -DREFLECTCPP_UBJSON=ON -DREFLECTCPP_XML=ON -DREFLECTCPP_TOML=ON -DREFLECTCPP_YAML=ON
...
ubuntu@dev:~/reflect-cpp$ cmake --build build -j4
[0/2] Re-checking globbed directories...
[1131/1131] Linking CXX executable tests/yaml/reflect-cpp-yaml-tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for C++ modules, enabled using
REFLECTCPP_BUILD_MODULES. The main library is part ofrfl, while each serialisation format is a partition of the module (rfl:*).