-
Notifications
You must be signed in to change notification settings - Fork 44
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
References for existing structures #209
Conversation
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.
The code is nicely formatted and after going through it I believe it is fully functional. I am a bit concerned with the usage of cast from base class to a derived class as you can read above. Please elaborate on that and tell me what you think. Otherwise it seems very good, thank you.
src/types/modules/module.cpp
Outdated
case Symbol::Type::Array: | ||
case Symbol::Type::Dictionary: | ||
{ | ||
auto structElement = ((IterableSymbol*)base)->getStructuredElementType(); |
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.
Casting base to derived class is always risky even when you check that you deal with the particular derived class instance. The best is to avoid such needs with different design when possible and when in real need it is better to use dynamic cast here. Is dynamic cast appliable here? Can we use something safer than (<>*) cast?
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.
We need access to the derived class because we want to be able to get and set the attribute of an iterable or structure base. I've tried to think of a way to avoid conversion, but that would lead to a duplication of _addStruct()- there would be one for a structure base and the other for an iterable base.
That would make it difficult to maintain or extend the code, should we attempt similar in-depth changes to the modules like I did in this PR, since functions in this module are usually called in a complicated circular dependency manner and further splitting of functionality would only make it worse.
Thank you for pointing out the possibility of using a dynamic_cast, I've adjusted the code to reflect that.
src/types/modules/module.cpp
Outdated
switch (base->getType()) | ||
{ | ||
case Symbol::Type::Structure: | ||
((StructureSymbol*)base)->addAttribute(newAttribute); |
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.
Again quite a risky cast. In Yaramod we use static_pointer_cast
to cast from pointer to derived class to pointer to base class which is safe, but not appliable here. If I am not mistaken, so far we did not need to cast base class to derived class. Is there any way we would avoid it here?
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.
As explained above, the cast is needed, because iterables and structures use different functions to get and set their attributes. I've changed the cast here to the same dynamic cast as in the other example, let me know if it's okay
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 the changes, I think it is LGTM. Good job!
This new feature allows users to define attributes that reference an existing structure.
Use case:
Changes had to be made to _addStruct and other parts of modules.cpp. The most serious one was the predeclaration of a struct symbol in it's parent object. It's assigned as an attribute of it's parent before attributes of the struct itself are loaded.
Sample declaration of references in modules: