Fix the dereference to NULL

[gaoethan]

rmw_*_validation_result_string(validation_result) may return NULL,
and it's dereferenced by passing arg to NameValidationError, this
leads unexpected behaviors and NULL is deprecated to use in C++ now.

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

Connects to ros2/rmw#130

[clalancette]

While using the ternary operator isn't exactly wrong, it is definitely odd to be re-assigning a variable back to itself. I'd prefer a simple if (nullptr == validation_result_string) here. Also, we should probably have a slightly different string than ""; maybe something like "Unknown error".

The same goes for the rest of the calls below.

[gaoethan]

actually, I've ever tried to make the error string specific, but when the result is valid, it also return NULL which is different from the default NULL in some sense, so it's not accurate if we treat all the NULL to "unknown error", I prefer to tweak the rmw_*validate_result_string to get it more accurate, any comments ? thanks.

[clalancette]

Yeah, I think I agree that we should ensure that the rmw_*_validation_result_string should probably never return NULL. I'll make a similar comment over in ros2/rmw#130 , but let's leave this open for now just in case we come to a different conclusion over there.

[gaoethan]

please evaluate to unify the undefined case in rmw_*_validation_result_string or deal with separate code while using based on #ros2/rmw#130

[mikaelarguedas]

typo ?
This also need to be on the same line as the above closing curly bracket to pass the linters

[mikaelarguedas]

Also this else is not necessary as if we enter the if we throw an error so the previous code can be kept

[gaoethan]

yes, it's a typo (my bad), actually the else got changed to not on the same line after I ran ament_uncrusitify --reformat, let me check.

why I add the else is because it will return NULL if validation_result is RCL_TOPIC_NAME_VALID, it will be problematic for InvalidTopicNameError and InvalidServiceNameError

[mikaelarguedas]

why I add the else is because it will return NULL if validation_result is RCL_TOPIC_NAME_VALID, it will be problematic for InvalidTopicNameError and InvalidServiceNameError

My understanding is that this is taken care of by the if just before

        if (validation_result == RCL_TOPIC_NAME_VALID) {
          throw std::runtime_error("topic name unexpectedly valid");
        }

[mikaelarguedas]

@gaoethan This comment has not been addressed.
I think that the following snippet should be kept. If this evaluates to true the code below is unreacheable as we throw. If it doesnt we're sure we're not in the case of validation_result == RCL_TOPIC_NAME_VALID

        if (validation_result == RCL_TOPIC_NAME_VALID) {
          throw std::runtime_error("topic name unexpectedly valid");
        }

[gaoethan]

Actually, I have kept it, :) generally speaking, it has lower possibility to hit that and I put it in the else statement

[mikaelarguedas]

Not sure if what scenario that can happen but should we do something in the else case ? (case where the rcl node name is valid but the rmw node name is not) (same everywhere below)

[gaoethan]

this if is under umbrella of RCL_RET_NODE_INVALID_NAME and it's currently handling the cases of invalid rmw node name as you point :). or what you really mean the case that rcl node name is invalid but rwm node name is valid ?

[mikaelarguedas]

Indeed that's what I meant 😄

[gaoethan]

@mikaelarguedas now it's not sure when this will happen, I throw exceptions for the case of valid rmw with invalid rcl checking to allow the user to be aware of that for further investigation.

[mikaelarguedas]

except for one remaining comment this looks good to me

lots of things changed

[mikaelarguedas]

lgtm