review comments by GlynLeine · Pull Request #1 · Rythe-Interactive/reflection-experiments

GlynLeine · 2025-10-01T07:20:33Z

No description provided.

Changed code style for whole solution.

There is circular dependency error.

…n-experiments # Conflicts: # applications/reflection-experiments/src/reflection_parsers/ast_source_parser.cpp # applications/reflection-experiments/src/reflection_parsers/ast_source_parser.h

Still circular issue.

GlynLeine · 2025-10-21T13:42:44Z

applications/reflection-experiments/src/code_gen/reflection_code_generator.cpp

+
+}
+
+rsl::dynamic_string reflection_code_generator::get_gen_source_file(rsl::string_view source_location)


since this function is only used in the reflection_code_generator and static, it might be better to leave this as an implementation detail function, so a global function in an anonymous namespace, or marked static so it gets internal linkage

GlynLeine · 2025-10-22T12:27:05Z

...ons/reflection-experiments/src/compiletime_reflection_containers/compile_reflected_class.cpp

+{
+    rsl::id_type hash = rsl::internal::hash::default_seed;
+
+    this->compile_reflection_container<compile_reflected_class>::sort_container(


I think we mentioned that if the offset is taken into the hash, then it doesn't need to be sorted right?

yeah, but the hash itself is still an ordered ordeal, so before applying them to hash, we need to put them into correct order. If it was like in AST the hash would be different as AST itself does not guarantee source-like ordering, so we need to always hash in offset order.

GlynLeine · 2025-10-22T12:28:19Z

...ons/reflection-experiments/src/compiletime_reflection_containers/compile_reflected_class.cpp

+        compile_reflected_variable>::get_container_hash();
+    if(hash != 0) { hash = rsl::combine_hash(rsl::internal::hash::default_seed, hash, variable_container_hash); }
+
+    this->compile_reflection_container<compile_reflected_function>::sort_container(


I don't think the order of the functions matters in the structure hash, arguably functions don't matter at all for the structure hash

yeah, kinda make sense, but strictly from reflection id stand point, if we have a class that is the same in all but functions, id still say its a good idea to make it produce different hash. But for our reflection we still need to know the functions, so the difference is only do we hash functions, which id say we should do.

after giving it another thought, i also want to exclude functions from hashes, would be pretty strange if saves would fail cos you have added a function.

GlynLeine · 2025-10-22T12:30:41Z

...s/reflection-experiments/src/compiletime_reflection_containers/compile_reflected_element.cpp

 #include <ostream>

-compile_reflected_element::compile_reflected_element(rsl::dynamic_string&& name) : name(std::move(name)) {}
+compile_reflected_element::compile_reflected_element(rsl::dynamic_string name)


if you're gonna move, then take it in as a rvalue reference. if you also want to allow copying then also supply a copy constructor using a const lvalue reference. don't take a heavy object like a string in by forced copy value.

GlynLeine · 2025-10-22T12:33:03Z

...ations/reflection-experiments/src/compiletime_reflection_containers/compile_reflected_file.h

+
 private:
-    rsl::dynamic_string source_location;
+    const rsl::dynamic_string source_location;


if you make member variables const, be careful. it's good practice, but it also means that copy or move assignment is deleted. this type becomes construct only

GlynLeine · 2025-10-22T12:34:53Z

.../reflection-experiments/src/compiletime_reflection_containers/compile_reflected_variable.cpp

 #include "compile_reflected_variable.h"

-compile_reflected_variable::compile_reflected_variable(CXCursor cursor)
+compile_reflected_variable::compile_reflected_variable(CXCursor& cursor, CXCursor& parent)


these could be const&, i know the clang library takes them is as a forced value copy.... but i would not take that as a good example. looking at the size of CXCursor, I'd definitely pass it around as a reference. but since it is copied for every clang_ function anyways, we have no need for it to be non-const.

tru, i have probably forgotten to change this to const

GlynLeine · 2025-10-22T12:36:12Z

...eflection-experiments/src/compiletime_reflection_containers/compile_reflection_container.inl

-
 template<typename T>
-T& compile_reflection_container<T>::add_element(const CXCursor& cursor)
+T& compile_reflection_container<T>::add_element(CXCursor& cursor, CXCursor& parent)


can be const& for the same reason i mentioned earlier

GlynLeine · 2025-10-22T12:39:37Z

...eflection-experiments/src/compiletime_reflection_containers/compile_reflection_container.inl

+template<typename T>
+void compile_reflection_container<T>::verify_typename() const
+{
+    static_assert(std::is_base_of_v<compile_reflected_element, T>, "T must inherit from compile_reflected_element");


static asserts don't need to be called at runtime, so they don't even need to be in a function. they can exist anywhere in the class or other member functions. static_asserts get executed at compile time. the reason why this static assert works, and the concept restriction didn't, is because this function gets instantiated later, after the type T is complete.

Yep, i know that and in this case i wanted to do that from the start (put it in the body of class), but that didnt work, as static assert is called immideately when the class is encountered, so it still tried to use incomplete "compiletime_reflection_class", during the process of compilation. So in this case it seems to me that we had to put here.

GlynLeine · 2025-10-22T12:42:25Z

...ons/reflection-experiments/src/compiletime_reflection_containers/compile_reflected_class.cpp

+    rsl::id_type hash = rsl::internal::hash::default_seed;
+
+    this->compile_reflection_container<compile_reflected_class>::sort_container(
+        &compile_reflection_container<compile_reflected_class>::sort_by_offset_comparator);


arguably, contained classes don't matter for the structure hash either.... although it becomes tricky with anonymous structs and unions, so maybe rather safe than sorry and include them anyways... hmm....

GlynLeine · 2025-10-22T12:45:22Z

...eflection-experiments/src/compiletime_reflection_containers/compile_reflection_container.inl

+
+    for(std::size_t i = 0; i < min_length; ++i)
+    {
+        if(a_string[i] < b_string[i]) { return true; }


if(a_string[i] != b_string[i]) { return a_string[i] < b_string[i]; }

fair enough

GlynLeine · 2025-10-22T12:50:33Z

applications/reflection-experiments/src/reflection_id/reflection_id.cpp

+
+extern "C"
+{
+#include "xxhash.h"


you can use:

#define XXH_STATIC_LINKING_ONLY /* access advanced declarations */ #define XXH_IMPLEMENTATION /* access definitions */

and then you can add xxhash as a header only library, and then there would be no need for extern "C" either.

GlynLeine · 2025-10-22T12:51:50Z

applications/reflection-experiments/src/reflection_id/reflection_id.cpp

+// Vector is not sorted before getting to this function
+rsl::id_type reflection_id::generate_structure_hash(std::vector<std::pair<std::size_t, rsl::id_type>>& members) noexcept
+{
+    std::ranges::sort(


if the structure hash of the members already includes the offset, then sorting is not necessary

…n-experiments

NiZe42 and others added 30 commits August 28, 2025 10:36

wip

976709a

fix hashed_string, move notes.cpp to outside of the project

962a012

update rsl to get argh

649c171

Update rsl

ce30cac

Merge remote-tracking branch 'origin/main'

d85b8d5

update rsl

c6661fa

Merge branch 'main' of https://github.com/Rythe-Interactive/reflectio…

703a691

…n-experiments

added working AST

ad2f7c0

added parameters

a703465

rsl::cli_parser alias

5d4713e

broken codegen

0011ea3

Merge branch 'main' of https://github.com/Rythe-Interactive/reflectio…

2d88278

…n-experiments

wip

dada67b

update rsl

1211ae8

wip

fbe1f85

update rsl again

a989037

wip broken print

291ea76

update rsl, rbs, and set debug args in project

f0e3701

update rsl

e890759

small suggestion

ba17054

works for hashed string too

e236e0b

WIP

561eb14

started adding compile reflection

0323990

not compiling variable container

0b064a1

Started reworking parser yet again.

da5308d

Changed code style for whole solution.

changed some more code style, continued with restructure

f3a2294

some minor review notes

1269573

almost done ast_parser

9e913f5

There is circular dependency error.

Merge branch 'main' of https://github.com/Rythe-Interactive/reflectio…

617706a

…n-experiments # Conflicts: # applications/reflection-experiments/src/reflection_parsers/ast_source_parser.cpp # applications/reflection-experiments/src/reflection_parsers/ast_source_parser.h

added comments back in, fixed a little error

88878ba

Still circular issue.

NiZe42 added 5 commits October 16, 2025 15:42

implemented reviews

3746067

WIP hashing system

fc81672

WIP still broken hashing

7aa6a4c

finished hashing

5485e1d

added const to constructors

ade8f67

GlynLeine commented Oct 21, 2025

View reviewed changes

GlynLeine commented Oct 22, 2025

View reviewed changes

NiZe42 and others added 13 commits October 24, 2025 18:21

fixed hashing bugs

31f27f3

WIP

dc954b3

continued codegen

35ca05d

fix bug

733a099

Almost working code gen

9e1950c

Code generation

974f900

Fixed move bug

d5b27c9

almost working runtime reflection

9c90d89

small fix

7abbe89

fix msvc compilation

97b8509

little fixes

6112d37

Merge branch 'main' of https://github.com/Rythe-Interactive/reflectio…

dc20a8b

…n-experiments

add target_class2 back in

c435162


		}

		rsl::dynamic_string reflection_code_generator::get_gen_source_file(rsl::string_view source_location)

Conversation

GlynLeine commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants