review comments #1

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.

still a little bit broken

GlynLeine · 2025-10-03T10:51:47Z

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

+#include "../runtime_reflection_containers/reflected_variable.h"
+
+rsl::dynamic_string reflection_code_generator::generate_variable(
+    rythe::reflection_containers::reflected_variable parsed_variable)


prolly want a const& here

true and real

I meant const& as the input parameter, the return type can be left without const

GlynLeine · 2025-10-03T14:06:17Z

applications/reflection-experiments/src/runtime_reflection_containers/reflected_function.h

+    {
+    private:
+        rsl::hashed_string                    name;
+        rsl::dynamic_string                   current_namespace;


might be nicer for this to be a reflected_namespace pointer

will add a little bit later when revising runtime variables

GlynLeine · 2025-10-03T14:08:30Z

...ations/reflection-experiments/src/runtime_reflection_containers/runtime_reflected_function.h

+
+        reflection_id                           return_type_id;
+        rsl::dynamic_array<reflection_id>       parameters;
+        rsl::dynamic_array<rsl::dynamic_string> attributes;


If we require custom attributes to all share a custom_attribute base class, then this could be an array of custom_attribute pointers instead, which would probably be a bit more usefull than strings. at runtime we probably want to be able to call a function like: value.has_attribute<my_attribute>() and this function should not rely on string operations.

but then in the end, attributes are custom as you can have anything inside it. Then it would mean that in the end we are still working with strings. And also because custom attributes in my head should be stored added by value somehow so it could live on the stack, as we will (probably) need multiple of each attributes so having new delete for each owner of attributes does not seems like a good solution to me.

you don't need new or delete, you can have it be a static local variable in the generated function, and pass the pointer to that variable to this list of attributes

GlynLeine · 2025-10-03T14:10:34Z

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

-
+#include <rsl/string>
+
+struct reflection_id


I would expect an ID to be an integer, if this what a type struct like a reflection_type then it would be more likely to have a name and attributes. IDs are used to identify things, but string operations are incredibly expensive. comparing 2 integers of any size is a lot faster than comparing strings.

would be hashes

.../reflection-experiments/src/compiletime_reflection_containers/compile_reflection_container.h

applications/reflection-experiments/src/reflection_parsers/ast_source_parser.cpp

applications/reflection-experiments/src/reflection_properties/access_modifier.h

applications/reflection-experiments/src/runtime_reflection_containers/reflected_function.h

GlynLeine · 2025-10-03T15:23:07Z

applications/reflection-experiments/src/runtime_reflection_containers/reflected_variable.cpp

+    reflection_properties::acess_modifier   access_modifier,
+    bool                                    is_static,
+    bool                                    is_const,
+    bool                                    is_array,


something like is_array and array_size I'd argue is more a property of the type of the variable than the variable itself? potentially the same goes for const, size and alignment.

yeah, you are probably right, will change it later

applications/reflection-experiments/src/source.cpp

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

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.

Implemented templates for containers

fb29c5c

still a little bit broken

GlynLeine changed the title ~~reviewe comments~~ review comments Oct 1, 2025

NiZe42 added 3 commits October 1, 2025 22:22

wip printing, added base element class

10f00e9

working parser with printing

097dd6d

little print improvement

22a031a

GlynLeine commented Oct 3, 2025

View reviewed changes

NiZe42 added 8 commits October 9, 2025 09:47

started code_gen

005669b

added hashing

f3cc14a

WIP not working code gen

0023950

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 added 4 commits October 24, 2025 18:21

fixed hashing bugs

31f27f3

WIP

dc954b3

continued codegen

35ca05d

fix bug

733a099


		}

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

review comments #1

Are you sure you want to change the base?

review comments #1

Uh oh!

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!

NiZe42 Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

NiZe42 Oct 16, 2025 •

edited

Loading