Skip to content
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

Avoid using Dynamic_cast #2604

Closed
wants to merge 1 commit into from

Conversation

med-ayssar
Copy link
Contributor

The Model class provides a type_id field that can be used for applying type checking on the Node instance, and avoid using the dynamic_cast.

@terhorstd terhorstd added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 7, 2023
@terhorstd terhorstd added this to PRs in progress in Kernel via automation Feb 7, 2023
@terhorstd terhorstd removed this from PRs in progress in Kernel Feb 7, 2023
@terhorstd terhorstd added this to To do in Models via automation Feb 7, 2023
@terhorstd terhorstd requested review from clinssen, jougs and JanVogelsang and removed request for jougs February 7, 2023 11:26
Copy link
Contributor

@JanVogelsang JanVogelsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the fact that the wholemodelrange_manager design is quite an antipattern in itself, this change actually makes sense.

Node* vt = kernel().node_manager.get_node_or_proxy( vtnode_id, tid );
vt_ = dynamic_cast< volume_transmitter* >( vt );
if ( not vt_ )
std::string model_name = ClassToString( volume_transmitter );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the macro necessary; why can't we just use the const string literal "volume_transmitter"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you say it, using the macro is actually incorrect here, as the name of the class does not have to match the name of the model. This works as a workaround, but what we would actually need would be a way to retrieve the type_id of a model from the type, so a function of the form:
index ModelManager::get_type_id<volume_transmitter>();
However, in this case, we could just assume that volume_transmitter is also registered as "volume_transmitter", even though this isn't the cleanest way and might break the code if the registration was to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting this comment, and I was planning to implement a static function that stores a default name of the model, but then this function must be implemented in all models.

I think, this raises the issue that models should have default names regardless of the registered name.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand the purpose of this change. Why do you not want to use the dynamic cast? Isn't that C++'s built-in way of testing whether a pointer points to a given type? Or is there a problem when applying CopyModel to a volume transmitter?

@@ -31,6 +31,8 @@
// Includes from sli:
#include "dictdatum.h"

#define ClassToString( cls ) #cls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this macro work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just a matter of performance, but now that Charl mentioned the problem with these changes, I don't think this change makes sense anymore, even if it would change performance at all. Not that this part of the code would be performance-critical/-relevant at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro simply takes the input and interprets it as the actual characters that were given to that macro. So all it does is change volume_transmitter to "volume_transmitter".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can apply certain checks if the underlying Node object can be converted to volume_transmitter, we can avoid using the dynamic_cast and therefore avoid the runtime overhead used for the conversion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have to perform some profiling, though. All the overhead required for the manual type-checking might actually outweigh the overhead of the dynamic_cast, as calling the RTTI might actually not consume that much more time. The branching will be what costs us a lot of cycles as well, and the if will still persist with your solution. So in total there might not even be an advantage of doing manual type-checking here.

@med-ayssar med-ayssar closed this Feb 8, 2023
@jougs jougs moved this from To do to Done in Models Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Models
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants