Conversation
4d2bd17 to
321b94c
Compare
|
I think these are great, first 2 down and working through the 3rd now, but these are definitely useful (I'm learning reflection from the 3rd). Perhaps the files could be renamed with a number indicating the order of the tutorials?
The pedant in me finds it odd to start in the middle, go down and then go up in the file view in VSCode. |
RyanJK5
left a comment
RyanJK5
left a comment
There was a problem hiding this comment.
I just gave the tutorials an in-depth look, and I think they are very good! All of the information is accurate. I left some comments, mostly regarding additional pieces of the reflection tutorial worth acknowledging. Thanks for putting these together!
|
@RyanJK5 thanks for the detailed review comments. This is exactly what’s needed for foundational material. Thanks for taking the time. |
RyanJK5
left a comment
RyanJK5
left a comment
There was a problem hiding this comment.
Looks good to me now, thanks for making those changes!
Replaces -DXYZ_PROTOCOL_BUILD_REFLECTION_TUTORIAL=ON as the way to opt into building and testing tutorials/3_reflection.cc.
| } | ||
|
|
||
| TEST(ReflectionHelpers, ClassifiesConstCorrectly) { | ||
| constexpr auto read_candidates = |
There was a problem hiding this comment.
Personal I'd perfer concept constrained auto here because being unfamilar with this function, and with no explanation of its use, I had to go to C++ ref and read the documentation to be confident I know what was happening. I think the hint is a range is enough knowledge to skip this step
| constexpr auto read_candidates = | |
| constexpr std::ranges::range auto read_candidates = |
There was a problem hiding this comment.
If you need to look at another tutorial to understand this one, that's a BAD sign.
I'll amend the use of auto here and elsewhere.
| TEST(ReflectionHelpers, ClassifiesConstCorrectly) { | ||
| constexpr auto read_candidates = | ||
| std::define_static_array(members_named(^^Widget, "read")); | ||
| constexpr auto write_candidates = |
There was a problem hiding this comment.
Again concept-constrained auto
| constexpr auto write_candidates = | |
| constexpr std::ranges::range auto write_candidates = |
| for (std::meta::info parameter : parameters_of(member)) { | ||
| if (!first) result += ","; | ||
| first = false; | ||
| result += display_string_of(dealias(type_of(parameter))); |
There was a problem hiding this comment.
Ashame we don't have dynamic delimiters for range formatting yet. I feel like this is the line the magic happens on, but I have to wade through additional cognitive noise to get the magic... particularly in light of the example going on to then only use a single parameter per function. Perhaps add a multi-parameter overload or just simplify for one param
| } | ||
|
|
||
| TEST(ReflectionHelpers, SignatureDistinguishesOverloads) { | ||
| constexpr auto write_candidates = |
There was a problem hiding this comment.
I'm not so worried about adding concept-constrained auto once the user is exposed to how this method works from earlier examples
| // Adapted from | ||
| // https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p2996r13.html | ||
| // | ||
| // struct data_member_options { |
There was a problem hiding this comment.
I had to go to C++ ref to understand the significance of this (in part because there is no indication this is a std::meta type). I initially thought this was an example of a struct you were going to generate... Particularly as in count_ratio_specs it's constructed with {}.
There was a problem hiding this comment.
How can I make this clearly useful?
@RyanJK5, I added this comment in response to your suggestion but I think the reader is still missing some context. What should I add?
There was a problem hiding this comment.
How can I make this clearly useful?
@RyanJK5, I added this comment in response to your suggestion but I think the reader is still missing some context. What should I add?
I think the best way would to add a comment before the struct definition explaining what it is. Combining that with the explicit data_member_options in the lines below that @Twon mentioned in the other thread, it should convey the significance.
| consteval std::vector<std::meta::info> count_ratio_specs() { | ||
| std::vector<std::meta::info> specs; | ||
| // clang-format off | ||
| specs.push_back(data_member_spec(^^int, {.name = "count"})); |
There was a problem hiding this comment.
This, for me, would connect the significance of the std::meta::data_member_options definition above and tell me it's a std::meta type.
| specs.push_back(data_member_spec(^^int, {.name = "count"})); | |
| specs.push_back(data_member_spec(^^int, data_member_options{.name = "count"})); |
3 participants
I'm not entirely sure that these tutorials are necessary.
The act of writing them was useful and perhaps they will be useful to readers of the (increasingly novel and technical) code in #95.