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

Cleanup the error handling in rcl_node_init. #1099

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

clalancette
Copy link
Contributor

Only undo things that we've done before. Not only does this result in less code, it also avoids spurious warnings from trying to cleanup things that we haven't initialized.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

having the inverted water fall order to finalize with goto statement works and do only necessary finalization. on the other hand, it would be complicated to add the process afterwards not to miss the correct jump point for code maintenance. (this could easily go to memory leak issue.)

i would prefer the single failing jump point and check if what is necessary to call with current implementation.

rcl/src/rcl/node.c Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

i would prefer the single failing jump point and check if what is necessary to call with current implementation.

That's fair. While this definitely cleans some things up, maybe it isn't the best strategy (even if I find it to be the easiest to understand). Let me try something else here.

@clalancette clalancette marked this pull request as draft August 30, 2023 00:04
Only undo things that we've done before.  Not only does
this result in less code, it also avoids spurious warnings
from trying to cleanup things that we haven't initialized.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette force-pushed the clalancette/rcl-node-init-revamp branch from 54163ca to 434c780 Compare September 15, 2023 14:43
@clalancette
Copy link
Contributor Author

All right, I've totally revamped this to go another direction. In particular, I keep the fail handling as it was, but just make it a lot more careful about only undoing things that have already been done. I think this turns out pretty nice, should still remove some of the bogus warning messages we get, and still removes a bunch of code. Please take another look when you get a chance!

@clalancette clalancette marked this pull request as ready for review September 15, 2023 14:45
@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

CI(retry):

  • Linux-aarch64 Build Status

@clalancette clalancette merged commit 7b9c1ec into rolling Sep 20, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/rcl-node-init-revamp branch September 20, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants