-
-
Notifications
You must be signed in to change notification settings - Fork 11.4k
Add C++ module support #9092
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 #9092
Conversation
|
Hello, If it is a discoverability issue, I am happy to refer to this from more location (it's already in misc/cpp/) but I am not sure I want to take on maintenance of this now, considering I have no idea how C++ modules work nor of their potential inevitable drawback. I am happy to offer hosting @stripe2933 solution as part of https://github.com/dearimgui/ if you like. |
Yes, that would be appreciated. Edit: relevant PR can be found in kelteseth/arewemodulesyet#71 |
|
@ocornut From an API point of view having a static module file is easier to maintain than dynamically generating the module from a script, especially if it is C++ and not another language. I don't see the point of the added complexity of using a script, which has a greater reliability risk than manually producing the module). (If you look at other libraries with modules, basically all of them provide them as static files rather than a generated file.) If you are concerned about maintaining the symbols in two different files, the way it works is whenever you add a new function you can just write the using statement to re-export it from the module, as there is a using statement for each symbol in the library. |
|
I kinda agree with this sentiment, and I suppose the worse case if that if I forget something it may be caught up with. Mainly at this moment I am not ready to accept maintaining this myself, because I don't really know/understand what it would really entail in term of user support. As a rule of thumb, I am expecting that anything designed by the C++ committee is going to backfire at some point in time, so I would prefer if this was independently maintained first. Would you like to host your own copy in a public repository and consider maintaining it first? |
|
There's a big difference between your implementation and stripe2933's one: the later is including all individual flags/enums in the .ccpm file. Therefore it is bigger and I believe it justify being automated. Yours: stripe2933's one: Can you explain is the later is actually justified/required to create a module file? |
|
Those are the unscoped enum constants that are automatically all exported by just exporting the enum itself. We don't actually have to export those manually I could run it as an independent but still related repo for the time being and update it as necessary, for ease of building I considered adding CMake support because this is to my knowledge the simplest and most ubiquitous way to compile modular translation units but I don't want to add things that would otherwise bloat or break the philosophy of the library. |
Based on my test, it works with Clang but not with MSVC. I’m not fully aware of the exact C++ standard rules behind this, but to maximize compiler compatibility, using automated bindings seems to be the better approach. I will check GCC’s behavior later and update accordingly. |
|
Then why not just offer the modules as you have already provided here? This is far less overhead than dynamically generating the module each time. This seems to make the most sense to me. |
|
I applied the same changes you made on your own repository, but with the following notes:
|
I can't understand what you're saying. The overall workflow is:
|
|
I mean generating the module for each user, as opposed to simply providing a static module as a "what-you-see-is-what-you-get". Also, the overhead of the author simply updating the list of exported symbols each update is much less than that of maintaining a Python script as well as finding bugs whenever they inevitably occur. |
Using
Then how the application can initialize the backend code?
Module names and namespaces are orthogonal. Whether to make them the same is a matter of personal choice.
I agree with your points. By explicitly adding the However, since the imgui library author has mentioned that he is not fully comfortable with C++ modules yet and maintaining them in this state would be difficult, the best approach for us is to host the symbol-export file in an external repository, encourage many users to rely on it, and work toward stabilizing it so that it can eventually be included in the official repository. |
A module is designed to encapsulate the public API of the library, if they want to change it they can always
Again, are these parts of the library API? Implementation details aren't the responsibility of a library to make public, unless it decides these are public API that need to be maintained/documented to the same degree. If a consumer really needs to access the backend and insist on doing so through modules, they can do the exact same thing mentioned earlier of creating their own module to access said features, but those if those features were intended to be public they would not have been relegated to a file outside of the main header.
Whether there is a benefit of naming them identically is up for debate, but there is quite certainly no benefit to naming them differently. Most people would expect that importing
I think the best way to achieve library stability for such a feature is to provide it in such a way that its contents are glaringly obvious, rather than behind a script. I do not know whether the script includes external dependencies or not, but even if it does not we still provide far less work for the user by providing the module as-is rather than having them run a script to do so. |
|
@ocornut What are your thoughts on making library internals and backends accessible through modules, as opposed to just the main header? |
|
Sorry I accidentally posted an earlier answer before getting far enough in my thoughts, so discard the deleted post.
Backends are public API. I guess they are some sort of optional sub-libraries,
That makes sense, but note that some users of imgui_internal.h would frequently include it. Assuming modules are a required replacement for headers (due to either hard constraints, either perhaps C++ users zealotry/eagerness to use new stuff) then both internals and backends should be accessible the same way. If they are not a hard requirement then I'd assume it is easier to avoid it. It partly comes down to the question: what's the purpose of modules? Initially I guess they were aimed to improve compile-time (I've seen some reports that they eventually don't so much, the sorts of outcome I would expect from anything in modern C++ land). I'd be interested in an actual breakdown/explanation as to why you care or need modules in the first place? I also don't know how modules would affect the perception of API stability and access to The only thing I am sure about at this point is that providing a "output" repo with just the module files is going to be more attractive to the end user than forcing them to know about a generator. But if we decide it is worth emitting a module for internals then maybe a generator makes sense. |
|
Well, the problems modules solve (to my knowledge) are:
I'm building some projects using purely modules (and very few headers unless necessary), which is part of why I am personally interested in module adoption. I do not know of any situation where modules are strictly necessary and headers would not be, besides similar passion projects to mine where I constrain myself to modules. Realistically, for the time being most people are still using C++17 or maybe even earlier, so there is not much demand for most consumers to use modules, I only proposed my PR because I think that for users of C++20+ the option to access the library through modules would be a great idiomatic improvement, though it isn't a hard requirement. (There are similar cases in other languages, for example Java 9 introducing JPMS yet some libraries haven't adopted it yet, but I digress.) The In my personal opinion I think we should only modularise the In the end, I don't know if you planned to allow anyone to access Also, any thoughts on the module name? I think |
This is good idea, but I gave up it because the relationship of
module;
#include <imgui.h>
export module imgui;
export { ... } // export symbols in imgui.h
module;
#include <imgui_internal.h>
export module imgui:internal;
export { ... } // export symbols in imgui_internal.h
module;
#include <imgui_impl_glfw.h>
export module imgui:glfw;
export { ... } // export symbols in imgui_impl_glfw.hand end user just make the build system configuration file like:
add_library(imgui_module)
target_sources(imgui_module PUBLIC
FILE_SET CXX_MODULES
FILES imgui.cppm imgui_internal.cppm imgui_impl_glfw.cppm
)
target_link_libraries(imgui_module PRIVATE imgui::imgui)and want to import all of them just with diff --git a/imgui.cppm b/imgui.cppm
index 7fd68a0..25a7d6c 100644
--- a/imgui.cppm
+++ b/imgui.cppm
@@ -3,5 +3,7 @@ module;
#include <imgui.h>
export module imgui;
+export import :internal;
+export import :glfw;
export { ... } // export symbols in imgui.hTherefore modifying the module export file is unavoidable. Unfortunately, modifying the third party library file is cumbersome when using package manager (e.g. vcpkg). So I didn't adopted this model.
It is named as internal, but I perceive it as experimental. Anyway, the usage of
Not all libraries follow this convention. Vulkan-Hpp uses In my honest opinion, choosing |
I don't think I understand. Why not use build system settings or extra conditional compilation flags to modify these settings? Seeing as I now know that these backends are indeed "public API" libraries, it's common for anyone to want access to potentially all of those headers. There isn't a compilation slow-down from importing a large module, anyway, so that isn't a concern. But having to manually import every single module rather than just a single import statement for the library seems very un-idiomatic here. I am sure we can find a better alternative. I think it makes the most sense if we just provide one module. If we end up building all those other modules, then the module for module;
export module ImGui;
export import ImGui.Main; // <imgui.h>
export import ImGui.Internals; // <imgui_internals.h>
export import ImGui.Glfw; // <backends/imgui_impl_*.h>, ...
export import ImGui.Sdl;
export import ImGui.Vulkan;
// etc.For this, we don't even have to use partitions. Each module can remain a full module, that is just exported by the final
Not everything follows this convention (and to be honest I don't know why they added the |
This PR creates a C++ module for Dear ImGui, rather than using generated bindings (as I have seen some other PRs propose), which is simpler for most users. The README file is updated as well to mention this.