Refureku icon indicating copy to clipboard operation
Refureku copied to clipboard

Add `getCanonicalTypeName()` for Field and Variable types.

Open Flone-dnb opened this issue 1 year ago • 5 comments

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() for VariableBase class which means that both Field and Variable will have this function. Canonical type name is taken from kodgen::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 to VariableBase instead of Type.

Here is an example:

#pragma once

#include <string>
#include <vector>

#include "Foo.generated.h"

using namespace std;

class CLASS() Foo{
public:
    Foo() = default;
    virtual ~Foo() = default;

    FIELD()
    const vector<string> myValue;

    FIELD()
    string myString;

    Foo_GENERATED
};
File_Foo_GENERATED

For myValue this function will return std::vector<std::basic_string<char>> and for myString it will be std::basic_string<char>.

Due to the lack of knowledge of this project I was unable to compile RefurekuTests target, got lots of errors like:

TestClass.rfks.h(23, 232): [C2259] 'UniqueInheritedProperty': cannot instantiate abstract class (compiling source file D:\Downloads\Refureku\Refureku\Library\Tests\Src\TestClass.cpp)

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 for myValue will not be std::vector<std::basic_string<char>> but int and the same for string field if you don't include <string>. So when shouldLogDiagnostic is true we can see:

...
[Warning] ...h:16:18: error: use of undeclared identifier 'string'
[Warning] ...h:19:5: error: unknown type name 'string'
[Warning] ...h:21:5: error: unknown type name 'Node_GENERATED'
...

I think we should detect actual errors (like use of undeclared identifier 'string') and stop code generation and ignore errors like unknown type name 'Node_GENERATED'. For this I'm proposing this PR.

Flone-dnb avatar Aug 28 '22 18:08 Flone-dnb

Hello.

Thank you for your contribution, I'm a bit busy right now but will look into it ASAP.

jsoysouvanh avatar Aug 31 '22 09:08 jsoysouvanh

I've fixed tests target compilation, I had to remove Generated directory so that it will be regenerated using the new generator.

Flone-dnb avatar Aug 31 '22 16:08 Flone-dnb

The whole point of this PR is do get type information for non-reflected types, but because Type can incorrect for non-reflected types (for example matching std::string and std::vector will be true) I'm adding this functionality to variables instead of types. Although we can 100% work on improving this and eventually move this to the Type but right now this will solve my issue and allow me to continue development on my project that uses Refureku.

I will work on requested changes and will change the target branch to dev.

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).

Flone-dnb avatar Sep 02 '22 10:09 Flone-dnb

I consider this PR done. Although I also want you to merge my other PR.

Flone-dnb avatar Sep 02 '22 18:09 Flone-dnb

The whole point of this PR is do get type information for non-reflected types

I get your point here, but it sounds off in the first place wanting to get reflection data from an unreflected class. If you want to get reflection data, you should reflect your class in the first place, and it's a design decision on your side not wanting to do so.

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 should be able to work with your forked version until then.

have you though about using clang-format and/or clang-tidy for code-style?

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.

jsoysouvanh avatar Sep 03 '22 08:09 jsoysouvanh