Skip to content

Fix up C functions to never throw.#149

Merged
hidmic merged 3 commits intomasterfrom
hidmic/avoid-throws
Nov 10, 2020
Merged

Fix up C functions to never throw.#149
hidmic merged 3 commits intomasterfrom
hidmic/avoid-throws

Conversation

@hidmic
Copy link
Copy Markdown

@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>
@hidmic hidmic requested review from Lobotuerk and ahcorde September 30, 2020 16:53
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

6 participants