Make more of the Waitable API abstract#2593
Merged
clalancette merged 2 commits intorollingfrom Aug 2, 2024
Merged
Conversation
I initially started looking at this because clang was complaining that "all paths through this function will call itself". And it is correct; if an implementation does not override these methods, and they are every called, they will go into an infinite loop calling themselves. However, while looking at it it seemed to me that these were really part of the API that a concrete implementation of Waitable needed to implement. It is fine if an implementation doesn't want to do anything (like the tests), but all of the "real" users in the codebase override these. Thus, remove the implementations here and make these pure virtual functions that all subclasses must implement. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
In particular, these are implementations in the Waitable class that only throw exceptions. Rather than do this, make these APIs pure virtual so all downstream classes have to implement them. All of the current users (except for tests) do anyway, and it makes the API much cleaner. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Contributor
Author
|
CI:
|
alsora
approved these changes
Jul 31, 2024
Contributor
Author
|
New CI after fixing the system tests:
|
Contributor
Author
|
New CI after responding to feedback in ros2/system_tests#548:
|
fujitatomoya
approved these changes
Aug 1, 2024
Collaborator
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI
Collaborator
|
@clalancette windows failures seem unrelated, just in case can you take a look? |
Contributor
Author
|
The only failure here is on Clang, and there, this PR improves things. With that, I'm going to go ahead and merge in both this one and ros2/system_tests#548 . Thanks for the reviews! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I initially started looking at this because clang was complaining
that "all paths through this function will call itself" (see https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1966/clang/new/fileName.-934007317/). And it
is correct; if an implementation does not override these methods,
and they are ever called, they will go into an infinite loop
calling themselves.
However, while looking at it it seemed to me that these were really
part of the API that a concrete implementation of Waitable needed
to implement. It is fine if an implementation doesn't want to do
anything (like the tests), but all of the "real" users in the codebase
override these.
Thus, remove the implementations here and make these pure virtual
functions that all subclasses must implement.
Note that I've split this into two commits for a particular reason. 5b3f59e is the one that will fix the clang warning. But while I was looking at it, I noticed that there are some other methods in there that really should be pure virtual. So commit 0194fb7 isn't required, but I think it is better C++.
This also requires ros2/system_tests#548 to go in at the same time.