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

Fix up C functions to never throw. #149

Merged
merged 3 commits into from
Nov 10, 2020
Merged

Fix up C functions to never throw. #149

merged 3 commits into from
Nov 10, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 30, 2020

This will reduce coverage somewhat, as the conditions on which these exceptions are generated cannot be reproduced (yet).

CI up to rmw_implementation, test_rmw_implementation, and rcl:

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

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
Contributor

@Lobotuerk Lobotuerk left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@ahcorde
Copy link
Contributor

ahcorde commented Oct 13, 2020

CI up to rmw_implementation, test_rmw_implementation, and rcl:

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

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, @hidmic should we merge?

@iuhilnehc-ynos
Copy link
Contributor

iuhilnehc-ynos commented Nov 2, 2020

Catching the exception seems reasonable to me,
but I think to use const string& symbol_name instead of const char * symbol_name for lookup_symbol is not good, which causes an extra calling constructor of std::string.
I know SharedLibrary of rcpputils has a method

  RCPPUTILS_PUBLIC
  void *
  get_symbol(const std::string & symbol_name);

that the parameter type is std::string, but I think it's better to ask rcpputils to provide an extra method for SharedLibrary, such as

  RCPPUTILS_PUBLIC
  void *
  get_symbol(const char * symbol_name);

, rather than updating the parameter type for lookup_symbol in rmw_implementation.

What do you think about that?

@hidmic
Copy link
Contributor Author

hidmic commented Nov 2, 2020

@iuhilnehc-ynos that's reasonable, but it doesn't preclude the need for this patch. Unless we make those methods noexcept, there are other reasons (besides a failing std::string construction) for them to throw. We have to handle them all.

@hidmic
Copy link
Contributor Author

hidmic commented Nov 2, 2020

Running CI again, this time computing test coverage:

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

Also, for reference, a Linux build with test coverage but w/o this patch:

  • Linux Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Nov 10, 2020

Alright, from what I can observe in above's partial coverage computations, this patch does not appreciably degrade coverage. I'll go ahead and merge. Let's see what nightlies have to say.

@hidmic hidmic merged commit 6053f55 into master Nov 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/avoid-throws branch November 10, 2020 20:54
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.

None yet

6 participants