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.

Sign up for GitHub

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

Print warning if non-FQN namespace remapping passed #248

Merged
merged 7 commits into from
Jun 4, 2018
Merged

Print warning if non-FQN namespace remapping passed #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b50acaf
More descriptive error messages
dhood Jun 2, 2018
15da6d1
Special error message when lexeme not found
dhood Jun 2, 2018
ba4ee35
Output warning if ns remapping invalid
dhood Jun 2, 2018
d838bd1
Output warning is ns remapping is invalid but starts with /
dhood Jun 2, 2018
fd8fd58
Add index to 'not found' error message
dhood Jun 4, 2018
24385eb
Distinguish between NONE and EOF
dhood Jun 4, 2018
81aabf8
Firmer wording to avoid overlap in meaning
dhood Jun 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rcl/include/rcl/lexer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ extern "C"
/// Type of lexeme found by lexical analysis.
typedef enum rcl_lexeme_t
{
/// Indicates no valid lexeme was found
/// Indicates no valid lexeme was found (end of input not reached)
RCL_LEXEME_NONE = 0,
/// Indicates end of input has been reached
RCL_LEXEME_EOF = 1,
Expand Down
16 changes: 15 additions & 1 deletion rcl/src/rcl/arguments.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ rcl_parse_arguments(
if (RCL_RET_OK == _rcl_parse_remap_rule(argv[i], allocator, rule)) {
++(args_impl->num_remap_rules);
} else {
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "arg %d error '%s'", i, rcl_get_error_string());
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "arg %d (%s) error '%s'", i, argv[i],
rcl_get_error_string());
rcl_reset_error();
args_impl->unparsed_args[args_impl->num_unparsed_args] = i;
++(args_impl->num_unparsed_args);
Expand Down Expand Up @@ -656,6 +657,19 @@ _rcl_parse_remap_namespace_replacement(
}
ret = _rcl_parse_remap_fully_qualified_namespace(lex_lookahead);
if (RCL_RET_OK != ret) {
if (RCL_RET_INVALID_REMAP_RULE == ret) {
// The name didn't start with a leading forward slash
RCUTILS_LOG_WARN_NAMED(
ROS_PACKAGE_NAME, "Namespace not remapped to a fully qualified name (found: %s)", ns_start);
}
return ret;
}
// There should be nothing left
ret = rcl_lexer_lookahead2_expect(lex_lookahead, RCL_LEXEME_EOF, NULL, NULL);
if (RCL_RET_OK != ret) {
// The name must have started with a leading forward slash but had an otherwise invalid format
RCUTILS_LOG_WARN_NAMED(
ROS_PACKAGE_NAME, "Namespace not remapped to a fully qualified name (found: %s)", ns_start);
return ret;
}

Expand Down
9 changes: 8 additions & 1 deletion rcl/src/rcl/lexer_lookahead.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,15 @@ rcl_lexer_lookahead2_expect(
return ret;
}
if (type != lexeme) {
if (RCL_LEXEME_NONE == lexeme || RCL_LEXEME_EOF == lexeme) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@sloretz I added this when I noticed that parsing argv[0] was always giving:

[DEBUG] [rcl]: arg 0 (/home/dhood/ros2_ws/install_isolated/demo_nodes_cpp/lib/demo_nodes_cpp/talker) error 'Expected lexeme type 19, got 1 at index 77, at /home/dhood/ros2_ws/src/ros2/rcl/rcl/src/rcl/lexer_lookahead.c:227'

That was the EOF case; perhaps the NONE case actually shouldn't be treated the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

NONE might deserve separate handling since it means there was text left but there's no way it could be a valid lexeme. Adding the text index to the message would give enough info to differentiate the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, that's the difference, thanks. I tried to point it out in docs https://github.com/ros2/rcl/pull/248/files#diff-b5f802bb85e2a3c95df3c6bbfd75b826R33, but I admit to not being aware of the different ways in which they're used; if the distinction is not so simple, we can remove that comment

RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
buffer->impl->allocator, "Expected lexeme type (%d) not found, search ended at index %lu",
type, buffer->impl->text_idx);
return RCL_RET_WRONG_LEXEME;
}
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
buffer->impl->allocator, "Expected %d got %d at %lu", type, lexeme, buffer->impl->text_idx);
buffer->impl->allocator, "Expected lexeme type %d, got %d at index %lu", type, lexeme,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the motivation for a separate error message come from the difficulty of reading the lexeme types in the current message? If it output strings (TOKEN, NONE, EOF, etc) instead would one message for all cases be sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly just printing the type as a number instead of a string was the main motivation for separating the messages (because I had to go find the type to come to the conclusion that it wasn't found), but even printing strings I think it'd be clearer to the user to say explicitly that it wasn't found (as opposed to them having to infer it from the types). I also thought that it might deserve a different return code in some situations but that's just hypothetical :)

buffer->impl->text_idx);
return RCL_RET_WRONG_LEXEME;
}
return rcl_lexer_lookahead2_accept(buffer, lexeme_text, lexeme_text_length);
Expand Down