refactor: auto-merge symbol fields via Boost.Describe#1167
Conversation
|
| Scope | Lines Δ% | Lines Δ | Lines + | Lines - | Files Δ | Files + | Files ~ | Files ↔ | Files - |
|---|---|---|---|---|---|---|---|---|---|
| 🛠️ Source | 64% | 866 | 460 | 406 | 23 | 2 | 21 | - | - |
| 🧪 Unit Tests | 36% | 496 | 496 | - | 1 | 1 | - | - | - |
| Total | 100% | 1362 | 956 | 406 | 24 | 3 | 21 | - | - |
Legend: Files + (added), Files ~ (modified), Files ↔ (renamed), Files - (removed)
🔝 Top Files
- src/test/lib/Metadata/Merge.cpp (Unit Tests): 496 lines Δ (+496 / -0)
- src/lib/Support/Reflection/MergeReflectedType.hpp (Source): 281 lines Δ (+281 / -0)
- src/lib/Metadata/Symbol/Function.cpp (Source): 94 lines Δ (+19 / -75)
|
An automated preview of the documentation is available at https://1167.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-03-20 08:59:13 UTC |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1167 +/- ##
===========================================
+ Coverage 77.85% 78.26% +0.41%
===========================================
Files 346 348 +2
Lines 32459 32540 +81
Branches 6571 6525 -46
===========================================
+ Hits 25271 25468 +197
+ Misses 4776 4676 -100
+ Partials 2412 2396 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gennaroprota
force-pushed
the
refactor/auto_merge_symbol_fields_via_boost_describe
branch
2 times, most recently
from
52a375f to
5f54207
Compare
There was a problem hiding this comment.
The problem we have (we have / not your PR has) is that we can only use reflection in source files, but reflection is a compile-time feature, and it has much less value if we can’t use it to define the reflection operations.
For instance, this feature would typically be implemented as:
template <canMerge T>
void
merge(T& I, T&& Other)
{
// logic of mergeReflected(I, Other); directly
}only once.
But instead we have to define:
void
merge(EnumSymbol& I, EnumSymbol&& Other)
{
MRDOCS_ASSERT(canMerge(I, Other));
mergeReflected(I, Other);
}for each symbol that has this operation, because the information can't be available at compile time. We can't do it only once, and it's very repetitive (in fact, this is so repetitive that there could even be a DEFINE_MERGE macro to make this a little easier to maintain).
I can't say reflection is completely useless because we can still define mergeReflected in the source file. But it never lets us write code in a way that's easy to maintain with a single source of truth.
I think if the current direction is that we don't want public dependencies at all (which is not unreasonable), the best solution would be to bundle a single amalgamated Support/Describe.hpp file with the same contents of boost/describe.hpp we want iteratively include (with proper copyright notices, etc, the part we want removed, a short explanation of the changes: it could become MRDOCS_DESCRIBE in the headers until we have native reflection in C++, and so on). This is not different from what we've been doing for many other C++ features in this project.
Then we can create a single public custom Support/Reflection.hpp header with mrdocs operations for described objects, such as merge or whatever else (generators will probably need for_each operations, someone will probably need enum_to_string, and so on). With that, we can finally eliminate these workarounds and actually use reflection to remove code. So far, reflection is helping with correctness but not eliminating much code, and it's helping with maintenance costs.
Of course, the biggest concern then becomes the cost of maintaining this. I evaluated that, and Boost.Describe doesn't change significantly. It's been years. We're not using all of it, and the parts (on better, the connection) that depend on Boost.mp11 can be easily replaced with other simpler list types (like tuple or a short typedef) and just a couple of operations. Even if Boost.Describe changes significantly, which never happened, the probability that this is something that affects us is very low: we're just defining lists of pointers to member functions. Even the cost of maintaining it once every few years or creating it the first time is very low, because this is very repetitive code that AI would be really good at doing in about 20 minutes. Another consideration is that real C++ reflection will likely be available way before any of that can even happen, so we don't need to worry about what follows. The cost of maintaining N variants of each function we want "reflection" on is much higher than adjusting this once every few years, even if we have to.
Anyway, I think this is more of a comment on the state of reflection than on this PR. This PR is great as it is, considering our current constraints.
This replaces hand-written per-field merge logic with a reflection-based approach: mergeReflected() iterates all Boost.Describe'd base classes and members, applying a default type-based strategy via mergeByType(). The strategies are tried in order: bool |=, SymbolID take-if-invalid, ADL merge() dispatch (for types with custom semantics), Polymorphic<Type> take-if-placeholder, Polymorphic<Name> take-if-identifier-empty, .Implicit types take-if-implicit, Optional take-or-recursive-merge, enum take-if-zero, string take-if-empty, vector<T> dedup-append (when T has operator==), and vector<T> take-if-empty fallback. Every merge function now reduces to mergeReflected(I, Other). Types that need custom merge semantics define their own merge() overload, found via ADL: - merge(ExtractionMode) — leastSpecific() - merge(vector<Param>) — element-wise merge + append extras - merge(vector<FriendInfo>) — dedup by symbol ID - merge(Name) — auto-merge id and Identifier This refactor also fixed some bugs: - Symbol::Parent was merged backwards (took Other when I was valid) - RecordTranche::Usings was silently not merged - FunctionSymbol::IsRecordMethod was never merged - VariableSymbol::IsRecordField was never merged - FunctionSymbol::Attributes was never merged, unlike VariableSymbol::Attributes which was correctly dedup-merged The is_optional and is_vector traits are extracted into a shared ReflectionTypeTraits.hpp to avoid duplication between MapReflectedType.hpp and MergeReflectedType.hpp.
gennaroprota
force-pushed
the
refactor/auto_merge_symbol_fields_via_boost_describe
branch
from
5f54207 to
40bf69e
Compare
3 participants
No description provided.