-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add getCanonicalTypeName()
for Field and Variable types.
#15
base: dev
Are you sure you want to change the base?
Add getCanonicalTypeName()
for Field and Variable types.
#15
Conversation
Hello. Thank you for your contribution, I'm a bit busy right now but will look into it ASAP. |
I've fixed tests target compilation, I had to remove Generated directory so that it will be regenerated using the new generator. |
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.
Thank you for your contribution.
I have addressed a few comments for the nomenclature consistency.
I still have mixed feelings about having the canonical type name at the variable level to be honest though.
From a purely logical point of view, the data should be at the Type level, but in that case the canonical name may or may not be filled: if the type is used for a reflected field/variable or method/function parameter/returned type within the same translation unit, the type canonical name will be valid, otherwise it will stay empty (and let's be honest, this is a huge problem).
I do not have any better idea for now to have this information consistently at the Type level, but I will think about it.
By the way, is it possible for you to modify this pull request to target the dev branch instead of master? I usually have everything on dev and merge to master only for new releases, plus it allows to have the CI run before moving to master.
Refureku/Library/Include/Internal/Refureku/TypeInfo/Archetypes/StructImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Archetypes/StructImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Archetypes/StructImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Archetypes/StructImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Archetypes/StructImpl.inl
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Variables/VariableBaseImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Variables/VariableBaseImpl.h
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Variables/VariableBaseImpl.inl
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Internal/Refureku/TypeInfo/Variables/VariableBaseImpl.inl
Outdated
Show resolved
Hide resolved
Refureku/Library/Include/Public/Refureku/TypeInfo/Variables/VariableBase.h
Show resolved
Hide resolved
The whole point of this PR is do get type information for non-reflected types, but because I will work on requested changes and will change the target branch to Also, because I see lots of code-style mistakes that I made, have you though about using clang-format and/or clang-tidy for code-style? With them developers don't need to care about code-style because clang tools will do all of the formatting automatically (for example, on Ctrl+S press and/or in CI). |
I consider this PR done. Although I also want you to merge my other PR. |
I get your point here, but it sounds off in the first place wanting to get reflection data from an unreflected class. I will merge this to dev and think about an optimal solution before merging to master. Having these changes pushed to master and then modified afterwards will break the library API/ABI which I want to avoid.
I know it exists but have never taken the time to look into it yet, will definitely have to do so if it can handle all code-style for us. I wonder how far it can go. |
Here is why I've decided to do this: i've tried creating a custom type T that inherits from `rfk::Object` and storing it in as a field of some other type. This caused a parsing error saying that T is abstract. I've noticed that `rfk::Object`'s `getArchetype()` function is pure virtual and it's implemented in the generated files, but since on parsing step we don't have anything generated this causes the parser to show this error. To avoid this issue I've added a default implementation for `getArchetype()` that just returns a class with the name `NULL` and size of `int`. Since all generated files implement `getArchetype()` I don't think this commit will make any difference to other code.
Running the new parsing that fails on parsing errors, some test files don't include stuff that they use.
Running the new parsing that fails on parsing errors, some test files don't include stuff that they use.
I've added the `shouldFailCodeGenerationOnClangErrors` parameter to the `RefurekuTestsSettings.toml` with the value `false`.
I've added the `shouldFailCodeGenerationOnClangErrors` parameter to the `RefurekuSettings.toml` file with the value `false`.
Hello. As I described in one of my issues I needed something like this, so I've spend some time to dig into the source code and I think I'm close to implementing this feature.
This PR adds
getCanonicalTypeName()
forVariableBase
class which means that bothField
andVariable
will have this function. Canonical type name is taken fromkodgen::TypeInfo
class and added to generated files. Since non reflected types can be considered equal (while in reality it may be incorrect) I've put canonical type name toVariableBase
instead ofType
.Here is an example:
For
myValue
this function will returnstd::vector<std::basic_string<char>>
and formyString
it will bestd::basic_string<char>
.Due to the lack of knowledge of this project I was unable to compile
RefurekuTests
target, got lots of errors like:I've regenerated reflection for test files but was still getting these errors. Maybe you need to regenerate reflection in some special way?
Also, I've noticed that if you don't include
<vector>
type name formyValue
will not bestd::vector<std::basic_string<char>>
butint
and the same for string field if you don't include<string>
. So whenshouldLogDiagnostic
istrue
we can see:I think we should detect actual errors (like
use of undeclared identifier 'string'
) and stop code generation and ignore errors likeunknown type name 'Node_GENERATED'
. For this I'm proposing this PR.