Implement is valid methods #103

allenh1 · 2016-12-29T04:58:09Z

Working on #81 .

This PR adds the rcl_x_is_valid calls for:

publishers
subscriptions
services
clients

Connect to #81

allenh1 · 2016-12-29T05:05:17Z

Currently, there is a small regression I don't know how to fix.

Error in destruction of intra process rcl publisher handle: publisher implementation is invalid, at /home/allenh1/ros2_ws/src/rcl/rcl/src/rcl/publisher.c:161

This happens if you send a publisher (and, oddly, a subscriber) a SIGINT.

wjwwood · 2017-01-03T20:54:54Z

I think this might be related to this: #100

Basically one of the things we check is whether or not rcl_shutdown() has been called, and if so return an error code saying it is invalid. We may need to relax that check for "getters".

allenh1 · 2017-01-04T17:11:50Z

That definitely makes sense. Perhaps we could just catch SIGINT and forward it to a proper shutdown function. I'll experiment with that later today.

wjwwood · 2017-01-04T21:53:43Z

SIGINT is already being caught, it's one of the responsibilities of the language specific client libraries, e.g. rclcpp or rclpy. Here is rclcpp's version of this that is done in rclcpp::init:

https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/utilities.cpp#L111-L123

allenh1 · 2017-01-11T20:59:00Z

That looks really good to me.

But, other than the regression, do these changes make sense to you? Did I miss any checks?

I tried to conform with the macro calls used in each function's checking.

wjwwood

Also, missing from this list is client (the counterpart to service).

The other thing that these functions do not check, which I think should be included in validity checks, is whether or not rcl_shutdown has been called since they were created. For the setters and getters this maybe too strict to check, again see #100, but for some functions like rcl_publish it might be a reason to abort the function. See:

Checking that might require that the instance id is retrieved while creating the entity (publisher or subscription or whatever) and storing it to be checked later in certain functions.

wjwwood · 2017-01-11T21:29:11Z

rcl/src/rcl/publisher.c

@@ -94,10 +94,10 @@ rcl_publisher_init(
 rcl_ret_t
 rcl_publisher_fini(rcl_publisher_t * publisher, rcl_node_t * node)
 {
+  if (!rcl_publisher_is_valid(publisher)) return RCL_RET_PUBLISHER_INVALID;
+  else if (!rcl_node_is_valid(node)) return RCL_RET_NODE_INVALID;


We always use {} to enclose if like statements. You'll want to run our linters over this code to catch any other issues like this. You can either run the tests or you can just run the linters directly:

$ ament_uncrustify /path/to/ws/src/ros2/rcl $ ament_cpplint /path/to/ws/src/ros2/rcl

Uncrustify can also automatically fix the style with the --reformat option.

Got it -- I'll use the linter before I push in the future.

wjwwood · 2017-01-11T21:29:29Z

rcl/src/rcl/publisher.c

-  RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_INVALID_ARGUMENT);
-  RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);
-  if (publisher->impl) {
+   if (publisher->impl) {


Indentation is off here.

wjwwood · 2017-01-11T21:29:47Z

rcl/src/rcl/publisher.c

    RCL_SET_ERROR_MSG(rmw_get_error_string_safe());
    return RCL_RET_ERROR;
-  }
-  return RCL_RET_OK;
+  } return RCL_RET_OK;


return should be on its own line.

wjwwood · 2017-01-11T21:31:48Z

rcl/src/rcl/publisher.c

+  RCL_CHECK_ARGUMENT_FOR_NULL(publisher, false);
+  RCL_CHECK_FOR_NULL_WITH_MSG(publisher->impl,
+                              "publisher implementation is invalid",
+                              return NULL);


Please wrap all or none of the arguments. I'm not sure if the linter will automatically correct this. This is what I would do:

RCL_CHECK_FOR_NULL_WITH_MSG( publisher->impl, "publisher implementation is invalid", return NULL);

Or this:

RCL_CHECK_FOR_NULL_WITH_MSG( publisher->impl, "publisher implementation is invalid", return NULL);

This applies here and everywhere else that you made a change.

wjwwood · 2017-01-11T21:37:21Z

rcl/src/rcl/publisher.c

-    publisher->impl, "publisher is invalid", return NULL);
-  RCL_CHECK_FOR_NULL_WITH_MSG(
-    publisher->impl->rmw_handle, "publisher is invalid", return NULL);
+  if (!rcl_publisher_is_valid(publisher)) return NULL;


Unfortunately, I don't know if we can use the rcl_publisher_is_valid function you created here. The check has become stricter, before this it checked publisher and publisher->impl for NULL, but with this change it also checks that publisher->impl->rmw_handle is not NULL. That might be ok, but we recently ran into an issue which might indicate that this is too strict to simply access the data in the structure. See: #100

The new function makes sense in rcl_publish and rcl_publisher_fini, but for the "getters" (like this function and others) it might be too strict (again see #100).

Ok -- I just removed the extra check. Now it just checks that publisher->impl is not NULL. I also changed it to return false instead of NULL, because I'm not sure why I had it as NULL instead of false.

wjwwood · 2017-01-11T21:39:08Z

rcl/src/rcl/service.c

+                              return false);
+  RCL_CHECK_FOR_NULL_WITH_MSG(*service->impl->rmw_handle->service_name,
+                              "service name is invalid",
+                              return false);							  


What is this one checking? That the string isn't empty, i.e. the first character is not \0?

Also, why check the service name here, but not check the topic name in publisher and subscription? I'm not sure you need to check it anywhere. I don't see any way that it could end up invalid.

Ok - I'll remove those checks.

allenh1 · 2017-01-18T06:46:56Z

Sorry for disappearing for a little there (first week back at school). When I ran uncrustify, I got the following:

allenh1@host-147-10 ~/ros2_ws $ ament_uncrustify src/rcl/
Traceback (most recent call last):
  File "/home/allenh1/ros2_ws/install/bin/ament_uncrustify", line 6, in <module>
    from pkg_resources import load_entry_point
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 664, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 634, in _load_backward_compatible
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 3019, in <module>
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 3003, in _call_aside
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 3032, in _initialize_master_working_set
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 655, in _build_master
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 963, in require
  File "/usr/lib64/python3.5/site-packages/setuptools-32.2.0-py3.5.egg/pkg_resources/__init__.py", line 849, in resolve
pkg_resources.DistributionNotFound: The 'ament-uncrustify' distribution was not found and is required by the application

I assume this is going to be Gentoo specific. If you'd like, I'll see if I can't find a patch for this.

Update: same for the ament_cpplint

allenh1 · 2017-05-16T22:44:39Z

@wjwwood Resolved conflicts. Am I missing anything else with this?

wjwwood · 2017-05-16T22:51:21Z

@allenh1 not sure, I'll catch up on it as soon as I can now that you've had a fresh look at it.

One thing I can see is that there is not documentation on some of the new functions and the ones that have them just have a brief. If you don't have time to improve the docs I'll do it, but if you're looking for something to do I recommend taking some inspiration from the docs of the other functions and consider cases like what if NULL passed and in what cases is each of those considered invalid. I believe it would be best if at least those two categories of things were covered if not more.

allenh1 · 2017-05-18T00:23:22Z

ok -- will do.

I'm getting ready to head out of town, so I might be a little lazy about it, but I'll definitely bother you about it at some point next week!

allenh1 · 2017-06-01T20:18:24Z

@wjwwood Just got around to doing the documentation here. I also moved a check I saw in rcl_subscription_get_topic_name to the rcl_subscription_is_valid function (checks that the option pointer is not NULL).

wjwwood · 2017-06-02T00:37:13Z

rcl/src/rcl/subscription.c

@@ -186,13 +186,19 @@ rcl_subscription_get_rmw_handle(const rcl_subscription_t * subscription)
 bool
 rcl_subscription_is_valid(const rcl_subscription_t * subscription)
 {
+  const rcl_subscription_options_t * options = rcl_subscription_get_options(subscription);


Isn't this an infinite loop? rcl_subscription_get_options calls this function.

Uh -- wups. Let me just undo that then.

You could split rcl_subscription_get_options up like this:

return_type _subscription_get_options(params...) { // just return the options, no checks return options } return_type rcl_subscription_get_options(params....) { // do checks, like call is_valid return _subscription_get_options(params...); }

Then you could use the internal _subscription_get_options function in is_valid.

Or since it is so simple you could just duplicate the logic to extract the options in both places.

wjwwood · 2017-06-02T00:39:05Z

Looks like client is still missing, from my review above:

Also, missing from this list is client (the counterpart to service).

Thanks for working on it 😄!

allenh1 · 2017-06-10T01:13:44Z

@wjwwood I pushed the fix for the rcl_subscription_get_options function if you wanna look at it. Tested it and it works (as far as I can tell).

Next on my list is the client checks. After that, we should be good right?

wjwwood · 2017-06-10T01:19:53Z

Tested it and it works (as far as I can tell).

Have you run all of the tests (even for downstream packages)? If not I can show you how or we can start a CI job to test it for you.

Next on my list is the client checks. After that, we should be good right?

When you've got that, I'll do another code review, and we'll run CI. If both of those don't turn anything up, then yeah I think this should be done.

wjwwood · 2017-06-10T01:59:47Z

I started some CI for you:

Linux
Linux-aarch64
macOS
Windows

allenh1 · 2017-06-22T07:04:52Z

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2017-06-22T07:11:44Z

@allenh1 If you select the arrow and select "rebase and merge" you will see that this conflicts

Edit: arg edited my comment rather than posting a new one 😖 . Ok I see that you pushed a branch of the same name on this repo but did not update the branch from where this PR is made. That's why this one still conflicts. I guess you can just force push on your fork to update this PR

allenh1 · 2017-06-22T07:15:27Z

@mikaelarguedas I already did so, and it was definitely not a good time.

I did, however, forget which remote I was working on. Let me force push the changes to the other one.

@mikaelarguedas I too edited my comment XD

wjwwood

There are a few things that remain to be addressed. Also since I've seen a few tricky cases which aren't being covered (like accessing something that might be NULL), please add tests for these functions which test each of the cases so we can ensure they're correct and not just rely on review.

wjwwood · 2017-06-22T07:20:10Z

rcl/include/rcl/client.h

@@ -370,6 +375,8 @@ RCL_WARN_UNUSED
 rmw_client_t *
 rcl_client_get_rmw_handle(const rcl_client_t * client);

+bool


Needs docs still.

wjwwood · 2017-06-22T07:22:36Z

rcl/src/rcl/subscription.c

+bool
+rcl_subscription_is_valid(const rcl_subscription_t * subscription)
+{
+  const rcl_subscription_options_t * options = _subscription_get_options(subscription);


This will fail if subscription is NULL.

This is very true. Thank you.

allenh1 · 2017-06-22T07:27:51Z

@wjwwood Got it -- will do.

…with a call to the function in the subscription code.

wjwwood

I did another pass on this. I found a few more things/questions to address.

I'd say we should address these new comments before running CI again.

Sorry it took so long for me to review it again... Still digging out of emails.

wjwwood · 2017-07-15T02:05:53Z

rcl/include/rcl/client.h

@@ -339,6 +339,11 @@ RCL_WARN_UNUSED
 const rcl_client_options_t *
 rcl_client_get_options(const rcl_client_t * client);

+///  Non-safe get_options function.
+/* *INDENT-OFF* */
+#define _client_get_options(client) &client->impl->options;


Does this need to be in the header? It would be better to put it in the .c file if possible. It shouldn't be part of the public API if possible (even if it has the leading _).

Same applies to the similar functions in all the other headers of this pr.

Also, there is some inconsistency in that this kind of function is in client, pub, and sub, but not service. Which is strange.

wjwwood · 2017-07-15T02:12:34Z

rcl/include/rcl/client.h

+ * The bool returned is `false` if:
+ *   - the argument `client` is `NULL`
+ *   - the argument's option pointer is `NULL`
+ *   - the argument's mw implementation is `NULL`


typo mw -> rmw

wjwwood · 2017-07-15T02:14:39Z

rcl/include/rcl/publisher.h

+/**
+ * The bool returned is `false` if:
+ *   - the argument `publisher` is `NULL`
+ *   - the argument's `publisher->impl` is `NULL`


The list of reasons for client is different (more exhaustive). I don't know if we need the list to be that exhaustive, but it ought to be consistent amongst similar elements.

Is this sufficient?

/** * The bool returned is `false` if the argument `publisher` is invalid. * The bool returned is `true` otherwise. */

(not leaving out the details beneath, but I didn't retype them here for clarity)

That's fine. As long as it is consistently done on each of the entities.

wjwwood · 2017-07-15T02:15:24Z

rcl/include/rcl/subscription.h

@@ -307,6 +307,10 @@ RCL_PUBLIC
 RCL_WARN_UNUSED
 const rcl_subscription_options_t *
 rcl_subscription_get_options(const rcl_subscription_t * subscription);
+/// unsafe _subscription_get_options


Again, here but not every where. Should be removed here or added everywhere else.

wjwwood · 2017-07-15T02:23:27Z

rcl/src/rcl/client.c

+  const rcl_client_options_t * options;
+  RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());        
+  options = _client_get_options(client);
+  RCL_CHECK_FOR_NULL_WITH_MSG(options, "client is invalid", return false, options->allocator);


If options is null, then you shouldn't access it to get the allocator. You should use rcl_get_default_allocator() here and optionally you could use options->allocator in the next null check.

That's a very good point. Not sure why I thought that would be a good idea.

No worries, probably a copy-paste error or something.

wjwwood · 2017-07-15T02:24:49Z

rcl/src/rcl/publisher.c

-    publisher->impl, "publisher is invalid", return RCL_RET_PUBLISHER_INVALID, options->allocator);
-  if (rmw_publish(publisher->impl->rmw_handle, ros_message) != RMW_RET_OK) {
-    RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), options->allocator);
+  } else if (rmw_publish(publisher->impl->rmw_handle, ros_message) != RMW_RET_OK) {


This doesn't need to be an else if since the previous if returns.

wjwwood · 2017-07-15T02:28:59Z

rcl/src/rcl/service.c

+    return false,
+    rcl_get_default_allocator());
+  return true;
+}


Why does this one not check the options of the service like it does for client and pub/sub?

wjwwood · 2017-07-15T02:29:35Z

rcl/src/rcl/subscription.c

+  RCL_CHECK_FOR_NULL_WITH_MSG(options,
+    "subscription implementation is invalid",
+    return false,
+    options->allocator);


Again, you shouldn't use options->allocator if options is null...

allenh1 · 2017-07-15T02:36:10Z

@wjwwood Thanks! I'll get to these shortly.

wjwwood

The code changes lgtm now. Thanks for iterating on it.

It would still be good to have some tests for these new "is valid" functions. That way we don't have to rely on my code review alone and so we can make sure the desired behavior doesn't accidentally get changes later.

But go ahead and check the linters locally and then run CI if you want.

allenh1 · 2017-07-17T22:23:46Z

After linting and whatnot, I ran the tests and hit the following:

2: Test command: /usr/bin/python3 "-u" "/home/allenh1/ros2_ws/install/share/ament_cmake_test/cmake/run_test.py" "/home/allenh1/ros2_ws/build/rcl_lifecycle/test_results/rcl_lifecycle/test_multiple_instances.gtest.xml" "--output-file" "/home/allenh1/ros2_ws/build/rcl_lifecycle/ament_cmake_gtest/test_multiple_instances.txt" "--command" "/home/allenh1/ros2_ws/build/rcl_lifecycle/test_multiple_instances" "--gtest_output=xml:/home/allenh1/ros2_ws/build/rcl_lifecycle/test_results/rcl_lifecycle/test_multiple_instances.gtest.xml"
2: Test timeout computed to be: 60
2: -- run_test.py: invoking following command in '/home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle':
2:  - /home/allenh1/ros2_ws/build/rcl_lifecycle/test_multiple_instances --gtest_output=xml:/home/allenh1/ros2_ws/build/rcl_lifecycle/test_results/rcl_lifecycle/test_multiple_instances.gtest.xml
2: Running main() from gtest_main.cc
2: [==========] Running 1 test from 1 test case.
2: [----------] Global test environment set-up.
2: [----------] 1 test from TestMultipleInstances
2: [ RUN      ] TestMultipleInstances.default_sequence_error_unresolved
2: 
2: >>> [rcutils|error_handling.c:148] rcutils_set_error_state()
2: This error state is being overwritten:
2: 
2:   'options argument is null, at /home/allenh1/ros2_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:255'
2: 
2: with this new error message:
2: 
2:   'could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104'
2: 
2: rcutils_reset_error() should be called after error handling to avoid this.
2: <<<
2: /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/test/test_multiple_instances.cpp:111: Failure
2: Value of: ret
2:   Actual: 1
2: Expected: 0
2: could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104
2: 
2: >>> [rcutils|error_handling.c:148] rcutils_set_error_state()
2: This error state is being overwritten:
2: 
2:   'could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104'
2: 
2: with this new error message:
2: 
2:   'options argument is null, at /home/allenh1/ros2_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:255'
2: 
2: rcutils_reset_error() should be called after error handling to avoid this.
2: <<<
2: 
2: >>> [rcutils|error_handling.c:148] rcutils_set_error_state()
2: This error state is being overwritten:
2: 
2:   'options argument is null, at /home/allenh1/ros2_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:255'
2: 
2: with this new error message:
2: 
2:   'could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104'
2: 
2: rcutils_reset_error() should be called after error handling to avoid this.
2: <<<
2: /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/test/test_multiple_instances.cpp:113: Failure
2: Value of: ret
2:   Actual: 1
2: Expected: 0
2: could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104
2: 
2: >>> [rcutils|error_handling.c:148] rcutils_set_error_state()
2: This error state is being overwritten:
2: 
2:   'could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104'
2: 
2: with this new error message:
2: 
2:   'options argument is null, at /home/allenh1/ros2_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:255'
2: 
2: rcutils_reset_error() should be called after error handling to avoid this.
2: <<<
2: 
2: >>> [rcutils|error_handling.c:148] rcutils_set_error_state()
2: This error state is being overwritten:
2: 
2:   'options argument is null, at /home/allenh1/ros2_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:255'
2: 
2: with this new error message:
2: 
2:   'could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104'
2: 
2: rcutils_reset_error() should be called after error handling to avoid this.
2: <<<
2: /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/test/test_multiple_instances.cpp:115: Failure
2: Value of: ret
2:   Actual: 1
2: Expected: 0
2: could not free lifecycle com interface. Leaking memory!
2: , at /home/allenh1/ros2_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:104
2: [  FAILED  ] TestMultipleInstances.default_sequence_error_unresolved (3 ms)
2: [----------] 1 test from TestMultipleInstances (3 ms total)
2: 
2: [----------] Global test environment tear-down
2: [==========] 1 test from 1 test case ran. (3 ms total)
2: [  PASSED  ] 0 tests.
2: [  FAILED  ] 1 test, listed below:
2: [  FAILED  ] TestMultipleInstances.default_sequence_error_unresolved
2: 
2:  1 FAILED TEST
2: -- run_test.py: return code 1
2: -- run_test.py: verify result file '/home/allenh1/ros2_ws/build/rcl_lifecycle/test_results/rcl_lifecycle/test_multiple_instances.gtest.xml'
2/8 Test #2: test_multiple_instances ..........***Failed    0.08 sec

I take this to mean that the checks I implemented are too aggressive. Should I be checking the options pointer?

wjwwood · 2017-07-17T22:25:57Z

I take this to mean that the checks I implemented are too aggressive.

Maybe, I'm not sure. It could also be that these tests or the implementation of the lifecycle stuff is making an incorrect assumption. You'll just have to dig into the test failures and see what needs to change.

Should I be checking the options pointer?

I think so, but that's just my impression without looking at it in detail.

allenh1 · 2017-07-17T22:58:26Z

Found it. It was an unnecessary check in the publisher's fini function.

Linux
Linux-aarch64
macOS
Windows

allenh1 · 2017-07-17T23:07:11Z

(I just changed one of the strings that said "subscription" in the service.c file to say "service" -- don't think this is worth re-triggering CI over)

@wjwwood can I merge this after the CI is green?

dirk-thomas · 2017-07-17T23:13:26Z

rcl/src/rcl/client.c

+{
+  const rcl_client_options_t * options;
+  RCL_CHECK_ARGUMENT_FOR_NULL(
+    client, RCL_RET_INVALID_ARGUMENT, rcl_get_default_allocator());


RCL_RET_INVALID_ARGUMENT? This function can only return bool.

Good catch! I believe that would have been the same as return 1, which is very not correct.

dirk-thomas · 2017-07-17T23:15:31Z

rcl/src/rcl/publisher.c

+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    publisher->impl, "publisher is invalid", return false, options->allocator);
+  RCL_CHECK_FOR_NULL_WITH_MSG(
+    publisher->impl->rmw_handle, "publisher is invalid", return false, options->allocator);


It would be better if the different checks use a different error message. Otherwise the user won't be able to distinguish the error case without additional debugging.

Same for the other *_is_valid functions.

That's a good point. I'll update other files as well.

dirk-thomas · 2017-07-17T23:28:42Z

rcl/include/rcl/client.h

+ *
+ * \param[in] client pointer to the rcl client
+ * \return `true` if `client` is valid, otherwise `false`
+ */


Should the docblocks mention that in case these functions return false an error message is being set?

I have no idea -- @wjwwood what do you think?

We normally don't mention that an error message is set, but in this case I think it makes sense because we're not returning an error code. I think we mention it explicitly in a few other functions where a pointer is returned (an error is set if NULL is returned instead). Can't hurt.

Will the user need to reset the error message before being able to call the next function "safely"?

Well it's always safe to leave the error message, but you do need to do it before calling something else if you don't want the overwriting an error message print. Elsewhere it is implied that a non RCL_RET_OK return code means an error message is set. Here it's not so clear that one would be set by a false return value. So I think it's appropriate.

Ok. I just added a simple

/** * In the case where `false` is to be returned, an * error message is set. */

Should I re-run the CI?

Are we expecting users to call these rcl_x_is_valid functions? If no, I think the current state documenting that an error message is being set in the case the functions return false is good.

If yes, I am not sure if that is a convenient API. I can imagine many cases where the caller doesn't care about the specific error message and will either have to write boiler plate code around every call to reset the error message or even forget about it (which will result in the error message being printed when attempted to be overwritten). Maybe in that case the functions should have a flag to optionally not set (or internall reset) the error message.

My understanding is that these functions are just to clean up the code so that, if a check is added in the future, it only needs to go in one place.

But that may be completely off-base, so I'll defer to @wjwwood.

I think the interface is fine as-is. I don't expect people to use these functions often (only client library implementers) and I expect even fewer cases where a return of false doesn't trigger an error case or at least a logging event. Also resetting the error is a single line, so it's not much boiler plate.

Having another option to not set the error seems unnecessarily complicated for the number of times it would be used.

Sounds good to me than. 👍

).

mikaelarguedas · 2017-07-18T17:19:10Z

rcl/include/rcl/client.h

+ * \return `true` if `client` is valid, otherwise `false`
+ */
+bool
+rcl_client_is_valid(const rcl_client_t * client);


Nit: missing empty line here

allenh1 · 2017-07-18T17:54:16Z

Ok, here's the new CI.

(updated, after linting)

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2017-07-18T20:30:31Z

I added an issue to add tests for these functions in the future: #150

wjwwood reviewed Jan 11, 2017

View reviewed changes

mikaelarguedas added the in review Waiting for review (Kanban column) label Apr 10, 2017

mikaelarguedas assigned wjwwood Apr 10, 2017

wjwwood reviewed Jun 2, 2017

View reviewed changes

allenh1 added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jun 22, 2017

wjwwood requested changes Jun 22, 2017

View reviewed changes

allenh1 mentioned this pull request Jun 27, 2017

ensure node is valid before using it #148

Merged

Hunter L. Allen added 4 commits July 7, 2017 13:03

Implemented service is valid check.

ee29a6e

Added an is-valid check for publishers.

3dd2053

Changed the checks to the single call to

e4ee3e1

Added subscription is_valid function, and replaced individual checks …

63d7339

…with a call to the function in the subscription code.

allenh1 added 4 commits July 7, 2017 13:03

Compiling is important

a2c82c7

Linters

905a4b3

Ok, tests passing. Running CI.

a8c844f

Changed subscriber check order, as well as added documentation.

a48635d

wjwwood requested changes Jul 15, 2017

View reviewed changes

wjwwood approved these changes Jul 17, 2017

View reviewed changes

Addressing new review

00c10c4

allenh1 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 17, 2017

dirk-thomas reviewed Jul 17, 2017

View reviewed changes

allenh1 added 3 commits July 17, 2017 16:37

Made the error strings more specific (addresses comments by @dirk-thomas

5e97020

).

Added a statement to clarify that a message is being set.

eb729a8

Fixed compiler warning.

10b422c

mikaelarguedas reviewed Jul 18, 2017

View reviewed changes

Revert white space changes

4728753

Re-linted.

d04b134

allenh1 merged commit af03d0c into ros2:master Jul 18, 2017

allenh1 removed the in review Waiting for review (Kanban column) label Jul 18, 2017

allenh1 deleted the 81-implement-is-valid-methods branch July 18, 2017 20:12

wjwwood mentioned this pull request Jul 18, 2017

add tests for rcl_*_is_valid() functions #150

Closed

ivanpauno pushed a commit that referenced this pull request Jan 2, 2020

rename {c_}utilities to rcutils (#103)

d88cdb1

Implement is valid methods #103

Implement is valid methods #103

Conversation

allenh1 commented Dec 29, 2016 • edited Loading

allenh1 commented Dec 29, 2016

wjwwood commented Jan 3, 2017

allenh1 commented Jan 4, 2017

wjwwood commented Jan 4, 2017

allenh1 commented Jan 11, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented Jan 18, 2017 • edited Loading

allenh1 commented May 16, 2017

wjwwood commented May 16, 2017

allenh1 commented May 18, 2017 • edited Loading

allenh1 commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Jun 2, 2017

allenh1 commented Jun 10, 2017

wjwwood commented Jun 10, 2017

wjwwood commented Jun 10, 2017

allenh1 commented Jun 22, 2017

mikaelarguedas commented Jun 22, 2017 • edited Loading

allenh1 commented Jun 22, 2017 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented Jun 22, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented Jul 15, 2017

wjwwood left a comment

Choose a reason for hiding this comment

allenh1 commented Jul 17, 2017

wjwwood commented Jul 17, 2017

allenh1 commented Jul 17, 2017

allenh1 commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented Dec 29, 2016 •

edited

Loading

allenh1 commented Jan 18, 2017 •

edited

Loading

allenh1 commented May 18, 2017 •

edited

Loading

mikaelarguedas commented Jun 22, 2017 •

edited

Loading

allenh1 commented Jun 22, 2017 •

edited

Loading

dirk-thomas Jul 17, 2017 •

edited

Loading

allenh1 commented Jul 18, 2017 •

edited

Loading