Skip to content

Convert node_id to Node only when necessary #2606

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Convert node_id to Node only when necessary #2606

wants to merge 3 commits into from

Conversation

med-ayssar
Copy link
Contributor

No description provided.

@terhorstd terhorstd marked this pull request as draft February 7, 2023 11:27
@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
@github-actions
Copy link

github-actions bot commented Apr 9, 2023

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Apr 9, 2023
@JanVogelsang
Copy link
Contributor

@med-ayssar What is the current state of this PR? If you give me a short summary of the changes and what is left to finish this PR, I am absolutely willing to continue this one for you.

@med-ayssar
Copy link
Contributor Author

@med-ayssar What is the current state of this PR? If you give me a short summary of the changes and what is left to finish this PR, I am absolutely willing to continue this one for you.

In some cases, the NodeId and Node* are not used correctly. You would always see the pattern that a NodeId was converted to Node* and the pointer was simply passed to the function without calling any function on the node, but instead they call the Node::get_node_id. That pattern was so annoying, and I was trying to remove some of them, but it was happening almost everywhere, and it was even more annoying to individually decide if the use of that pattern was correct or not.

@JanVogelsang
Copy link
Contributor

I see, but are the changes that you have done already better than no changes? So should we just merge this even though there are still occurrences left of that antipattern?

@github-actions github-actions bot removed the stale Automatic marker for inactivity, please have another look here label Jul 21, 2023
@JanVogelsang
Copy link
Contributor

Assigned @heplesser to check if it should be merged or if more work is needed here to turn this into an actual PR.

@JanVogelsang JanVogelsang mentioned this pull request Aug 24, 2023
Draft
3 tasks
@JanVogelsang JanVogelsang added the T: Discussion Still searching for the right way to proceed / suggestions welcome label Aug 24, 2023
@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@heplesser heplesser

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 stale Automatic marker for inactivity, please have another look here T: Discussion Still searching for the right way to proceed / suggestions welcome T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@med-ayssar @JanVogelsang @heplesser @terhorstd