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

Make get_thread virtual in Node class #2611

Closed
wants to merge 1 commit into from

Conversation

med-ayssar
Copy link
Contributor

The get_thread function in the Node class is not virtual, which makes the implementation of the function in the ProxyNode class useless. Mainly, the get_thread function in the ProxyNode class will never be called, as we always have a pointer of Node type pointing to the proxy instance.

My suggested change is to make the function virtual and remove the implementation from the ProxyNode.

@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 10, 2023
@terhorstd terhorstd added this to PRs in progress in Kernel via automation Feb 10, 2023
@terhorstd terhorstd added this to To do in Models via automation Feb 10, 2023
@jougs
Copy link
Contributor

jougs commented Feb 10, 2023

As discussed offline, this changes are problematic in that they

  1. add a vtable indirection for all calls to get_thread(), which might lead to a performance regression
  2. shadow the fact that get_thread() must not be called on a proxynode as it cannot return a sensible value for remote nodes represented by it.

I will thus close it.

@jougs jougs closed this Feb 10, 2023
Kernel automation moved this from PRs in progress to Done (PRs and issues) Feb 10, 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
Kernel
  
Done
Models
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants