Modularizing (#1586) #1599
Modularizing (#1586) #1599
Conversation
|
@stephenberry Do you have any thoughts on the code before I move on to actually supporting modules? Also do you want to try the new module structure or the current one is OK? |
|
Can you explain a little bit more the new approach? Does this have to do with internal macros to the library? Glaze does have some macros that are used across the library. Note that I noticed some weird handling of macros that are locally defined. See the modularization of module;
#include <array>
#define std_trait(x) static constexpr sv x = to_sv<std::x##_v<T>>()
#undef std_trait
export module glaze.api.trait;This |
|
The current approach is that we have a section for module specific stuff and a section for header specific stuff. The new approach is that we have 3 sections: shared, module, and header. This allows for less repeating stuff. For example, you can include a 3rd party header or Export.hpp only once, especially not duplicating standard includes. (Example in the link I gave). Functionality is exactly the same as this, just that it can generate shorter preambles. Also about the macro thing, keep in mind that this is just the output from the program, I didn't do any clean up yet. Though I try my best, my DCE pass is not so good. If you had an empty conditional macro, it will get eliminated, but I didn't think about the #define/#undef case lol. |
|
Thanks for your explanation. It sounds like the new approach is better. You'll probably be dropping the old approach, right? In which case I think it's better to move to the new. Yeah, I realize this is auto-generated from importizer. So, I figured I'd point out things that you can improve/fix. The #define/#undef is probably the best use of macros because it doesn't pollute anything, but can locally shorten code. |
|
I will work on those, and let's see how it goes. Right now, the biggest challenge is shortening the preamble in transitional mode. |
|
@stephenberry can you check it out? I implemented the shared code section as well as the #define/#undef removal. |
|
This is looking good. We should be able to add to the GitHub Actions and test building with modules. |
|
Std module status: If you want to use module;
#include <vector>
// Other standard includes
export module stdBootstrap;
using namespace std;
export namespace std {
using vector;
// using ...
}We get better compile time this way. We can swap out for compilers' standard modules once GCC is ready with some Ctrl+F. And you might have guessed it, I have a setting to do just this... |
|
Thanks for the info. I don't think An additional thought is that you might want to look into a way of handling git renaming so that the pull request doesn't show every line of every file as having changed. Git has methods to rename files so that changing to |
I will get to this at the very end. Are you ready for cleaning? Any last fixes? @stephenberry |
|
I don't see any problems, so I say keep going ahead! |
|
@stephenberry One more thing I forgot, glaze seems to have some headers depending on macros from other headers. While this would work in header build, it won't be in modules because modules don't leak macros. Local macros like GLZ_CSV_NL, DLL_EXPORT are fine. For glaze, there are a few way out: put the macros in a separate file, and include that file in those that needed the macros, put the macro definitions on the command line using -D. What do you want to do? I am seeing this happen with: GLZ_ALWAYS_INLINE, [most of the macros in util/parse.hpp] (what else?) |
|
I also replaced the |
Yes, this sounds fine. |
Yeah, the macros in
Note that C++ doesn't have a mechanism of optionally returning from an inlined function call. So, macros are needed to keep code reasonably clean when writing code without exceptions and needing to return on error. Hence, the reason for most of these macros. |
Can you elaborate further on this and give an example? |
|
Here's an example of a macro: #define GLZ_INVALID_END(RETURN) \
if constexpr (not Opts.null_terminated) { \
if (it == end) [[unlikely]] { \
ctx.error = error_code::unexpected_end; \
return RETURN; \
} \
}This can be used simply by adding the line: GLZ_INVALID_END();To do this with a forced inline function call we could write the function as: template <auto Opts>
GLZ_ALWAYS_INLINE void invalid_end(context& ctx, auto& it) noexcept {
if constexpr (not Opts.null_terminated) {
if (it == end) [[unlikely]] {
ctx.error = error_code::unexpected_end;
}
}
}To use this function we need to write: invalid_end<Opts>(ctx, it);
if (bool(ctx.error)) [[unlikely]] {
return;
}Not only is this more typing, but it actually creates two checks for the error rather than just one. So, it has double the branching. Even if it is optimized away in Release builds, it still makes Debug builds slower. And, the force inline tends to increase compilation times (especially on MSVC). So, the macro approach can build much faster. Now, consider functions that might have multiple return points. The optimizer begins to struggle more when the error paths become more complex. If we need to add this additional check after calling our function then we can end up with more branching than the macro code, and in order to force inline the function we also hurt compilation times. |
|
In looking at the error handling problem again I discovered a way to get as efficient assembly with the non-macro approach in Release mode. It looks like I can avoid using force inlining for at least the simpler functions. There will still be additional overhead in Debug builds, but I think it is worth it to eliminate macros. I'll do more experimentation later. |
|
I'm also curious about how you do this. Let me know so that I can update my part. |
|
Here's an example of using a boolean return that generates just as efficient assembly for GCC and Release. The important things are that I return a boolean and not an error code, and that the context error code is only set in the error path. #include <array>
#include <bit>
#include <concepts>
#include <cstddef>
#include <cstdint>
enum struct error_code : uint32_t
{
none,
expected_bracket,
expected_brace,
expected_quote
};
struct context
{
error_code error;
};
inline bool match_bracket(context& ctx, char*& it) noexcept
{
if (*it != '[') [[unlikely]] {
ctx.error = error_code::expected_bracket;
return true;
}
else [[likely]] {
++it;
return false;
}
}
void do_something(context& ctx, char*& it);
void func(context& ctx, char*& it)
{
if (match_bracket(ctx, it)) [[unlikely]] {
return;
}
do_something(ctx, it);
}
void func2(context& ctx, char*& it)
{
if (*it != '[') [[unlikely]] {
ctx.error = error_code::expected_bracket;
return;
}
else [[likely]] {
++it;
}
do_something(ctx, it);
} |
|
That was what I had in mind, too, yesterday. Returning a bool, use it as rvalue for RVO to kick in. Can you replace the macros so that I can update them on my end? @stephenberry |
|
Yes, I'll get to work on replacing macros. |
|
I'm removing macros in this pull request: #1614 |
|
Just making sure, @stephenberry, the only non-local macros left are in util/inline.hpp, right? Are there any other files that provides macro definitions for others? |
|
Yes, I think the only macros that need to be shared in order to build are those in |
|
I'm still trying to set up CMake modules build. It doesn't work at all, I need some help lol @stephenberry |
|
@msqr1, I'll look into this when I have a chunk of time. I'm a bit preoccupied by other work at the moment, but I am really excited about your work and eager to help. |
|
Hi, I got these error while trying to build normally with clang(++)-19 and libc++: In tests/beve_test/beve_test.cpp, just literally relace |
|
@msqr1, this first line tells the error: This would typically be a warning, but warnings are considered errors with |
More to be added, this is just a draft one. Modularized with importizer, code is not reviewed, fresh after running it through.
Regular build works perfectly, you can use the same command to build and test with zero changes. Working on exporting and writing the CMake portion for modules.