-
Notifications
You must be signed in to change notification settings - Fork 21
refactor(metadata): integrate Boost.Describe #1130
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: develop
Are you sure you want to change the base?
Conversation
|
| Scope | Lines Δ | Lines + | Lines - | Files Δ | Files + | Files ~ | Files ↔ | Files - |
|---|---|---|---|---|---|---|---|---|
| 🛠️ Source | 1974 | 1419 | 555 | 55 | 8 | 47 | - | - |
| 🤝 Third-party | 168 | 168 | - | 6 | 6 | - | - | - |
| ⚙️ CI | 42 | 41 | 1 | 1 | - | 1 | - | - |
| 📄 Docs | 8 | 8 | - | 1 | - | 1 | - | - |
| 🏗️ Build / Toolchain | 5 | 5 | - | 1 | - | 1 | - | - |
| Total | 2197 | 1641 | 556 | 64 | 14 | 50 | - | - |
Legend: Files + (added), Files ~ (modified), Files ↔ (renamed), Files - (removed)
🔝 Top Files
- src/lib/Support/Reflection/Reflection.cpp (Source): 527 lines Δ (+527 / -0)
- src/lib/Support/Reflection/MapReflectedType.hpp (Source): 239 lines Δ (+239 / -0)
- include/mrdocs/Dom/LazyObject.hpp (Source): 237 lines Δ (+140 / -97)
| #ifndef MRDOCS_API_METADATA_DOCCOMMENT_INLINE_IMAGEINLINE_HPP | ||
| #define MRDOCS_API_METADATA_DOCCOMMENT_INLINE_IMAGEINLINE_HPP | ||
|
|
||
| #include <boost/describe/class.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
includes should be ordered from most specific to most general. describe is general. std includes are most general
d8c7267 to
f3274f4
Compare
alandefreitas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 👏😀🎉
.github/workflows/ci.yml
Outdated
| uses: alandefreitas/cpp-actions/[email protected] | ||
| with: | ||
| source-dir: ../third-party/boost_describe | ||
| git-repository: https://github.com/boostorg/describe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the url is cheaper in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
| { | ||
| tag_invoke(t, io, I.asInfo(), domCorpus); | ||
| io.map("aliasedSymbol", I.AliasedSymbol); | ||
| mapWithDescribe(io, I, domCorpus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't mapWithDescribe just iterate the base classes itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it always iterate all bases? I was afraid there was some special case somewhere for which only one base was mapped. If you confirm I can iterate all bases, always, I'll do that :-).
After writing the above comment, I found this:
struct ImageInline final
: InlineCommonBase<InlineKind::Image>
, InlineContainer
{
/** Image source URL or path.
*/
std::string src;
/** Alternate text when the image cannot be shown.
*/
std::string alt;
/** Order images by source, alt text, and inline children.
*/
auto operator<=>(ImageInline const&) const = default;
/** Equality compares source, alt text, and contents.
*/
bool operator==(ImageInline const&) const noexcept = default;
};
template <class IO>
void
tag_invoke(
dom::LazyObjectMapTag t,
IO& io,
ImageInline const& I,
DomCorpus const* domCorpus)
{
tag_invoke(t, io, dynamic_cast<Inline const&>(I), domCorpus);
tag_invoke(t, io, dynamic_cast<InlineContainer const&>(I), domCorpus);
mapWithDescribe(io, I, domCorpus);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the general case, I can't think of any case in the library where we do not include information about base classes when describing the properties of an object. I can't think of anything like that happening anywhere, even hypothetically.
Sooner or later, this function will need options anyway, because the way it has to behave and some small details are always going to change from one kind of generator to the other. So if we find out that's the case at some point in the future, that's also not a problem.
Sorry. I forgot to answer this one. 😬 Yes. We're going to use canonical names for everything sooner or later. Since you already made the effort to split this into two tasks, we can do that later rather than sooner. Reflection will enable all kinds of simplifications in the code from now on. I leave it up to you, though. |
f3274f4 to
d3c9cbd
Compare
6bfe8ac to
394c55a
Compare
|
Sometimes we get conflicts if we don't rename things for Handlebars, as you notice with "class" and "functionClass", etc. This is because of how handlebars works with recursion, and it's very annoying. I don't know if it solves your problem, but one solution I was going to propose at some point is adding an "@meta" key to all objects to describe the class name, etc (this can also help with other problems we've been having). This way, we would have the proper information to make the distinction in handlebars. |
adf50c8 to
7485284
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1130 +/- ##
===========================================
+ Coverage 86.19% 86.25% +0.06%
===========================================
Files 323 326 +3
Lines 23954 23850 -104
===========================================
- Hits 20646 20573 -73
+ Misses 3308 3277 -31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ba65b54 to
45cde41
Compare
7a2018f to
ef512c3
Compare
… boilerplate This introduces Boost.Describe and Boost.Mp11 and applies them across metadata types (symbols, enums, inline elements) to replace hand-written io.map() calls with reflection-based code. This improves maintainability without altering the public API or user-facing features. Boost.Describe and Boost.Mp11 are *private* dependencies, i.e. they are not used in any public Mr. Docs include.
Reason: Avoiding timeouts in the Clang 21 + MSan tests.
|
An automated preview of the documentation is available at https://1130.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-09 18:27:27 UTC |
alandefreitas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really nice. Most comments I left are either compliments or suggestions for future issues. Regarding your question about generalizing the process and variable names, I would also leave that to another smaller issue.
| package-generators: ${{ matrix.mrdocs-package-generators }} | ||
| package-artifact: false | ||
| ctest-timeout: 3600 | ||
| ctest-timeout: 9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From 1 to 2.5 hours? What's happening here? In fact, 3600 was already a generous temporary workaround until we get some problems with tests fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. The Clang 21 + MSan test is usually taking between 1h 50m and 2h 10m. It's the only one to be so slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Well... I guess we can come up with some strategy for this later.
| git-repository: https://github.com/boostorg/describe | ||
| git-tag: boost-1.90.0 | ||
| patches: | | ||
| ./third-party/patches/boost_describe/CMakeLists.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
| { | ||
| MRDOCS_ASSERT(domCorpus); | ||
| mapReflectedType(io, I, domCorpus); | ||
| io.map("class", std::string("symbol")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to need another proper solution to this (another issue). Handlebars often need information about the class and so on, and these often clash with object members no matter what name we decide for the key. I was thinking of something like "@meta" where we would always provide a lazy representation of the class name, etc, to handlebars.
| doc::ImageInline const& I, | ||
| DomCorpus const* domCorpus) | ||
| { | ||
| tag_invoke(t, io, dynamic_cast<doc::Inline const&>(I), domCorpus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boost.Describe can also iterate the base classes in the function.
| DocComment const& I, | ||
| DomCorpus const* domCorpus) | ||
| { | ||
| io.defer("description", [&I, domCorpus] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we describe this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really nice. Is there a way to reduce compile time (or address another issue) so that any TU that includes this header doesn't have to parse all metadata-related headers? I don't know if there's a reasonable way to forward declare the struct descriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm pretty sure the BOOST_DESCRIBE_xxx() macros require a defining declaration. Or did you have something specific in mind to work around this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. Do you mean a forward declaration for the specializations those macros define is something that wouldn't work? I was thinking just of forward declarations so everything could be encapsulated. And of course we would replicate the macro that generates the forward declarations we need. Instead of completely describing something, we would just forward declare the classes that describing something generates. We could also have a macro that forward declares whatever classes describing would create. Doesn't that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not dug into the implementation details of Boost.Describe, but I guess it generates hidden internal structures that use pointers to members, and one cannot take a pointer to member of an incomplete type.
However, the current design already provides, I think, the separation you are asking about:
-
The struct definitions live in public headers.
-
The
BOOST_DESCRIBE_STRUCT()invocations live in Reflection.hpp, which is private (src/lib/). -
Only .cpp files in src/lib/ include Reflection.hpp. And there are only four of them (not one, but a small number anyway :-)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the requirements are met. My concern was only about compilation time. We can open an issue and just refer to the comment. We can investigate this later because it's not going to be that simple anyway.
356824f to
2d8d158
Compare
Reason: The more specific patterns should be cheaper to evaluate.
This introduces Boost.Describe and Boost.Mp11 and applies them across metadata types (symbols, enums, inline elements) to replace hand-written sequences of
io.map()calls with reflection-based code. This improves maintainability without altering the public API or user-facing features.@alandefreitas: Please have a look at
normalizeMemberName()in SymbolDescribeMapper.hpp: Some names are special-cased there. If you prefer, I can remove the special cases and modify the Handlebars templates to use the canonical names.