-
Notifications
You must be signed in to change notification settings - Fork 127
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.
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
perf: faster ament_libraries_deduplicate implementation #448
Conversation
236f107
to
1aee39a
Compare
@sloretz any update? |
@VRichardJP could you rebase this branch onto latest humble branch?(27ce75b) We will use your PR branch within the Autoware project through: |
3bbbe3c
to
8211da4
Compare
Hi @VRichardJP. Would you mind merging or rebasing to include the latest changes in the
The sign-off needs to match the commit author. |
8211da4
to
75329e7
Compare
@cottsay done! |
Looks like the tests I added for ament_libraries_deduplicate are failing. It appears this change modifies the behavior of the function under some conditions. We will need to either re-align the behavior or sufficiently explain why the new behavior is more correct. |
Signed-off-by: Vincent Richard <[email protected]>
75329e7
to
2065603
Compare
Nice catch Before: $ cmake -P ament_cmake_libraries/test/test_deduplicate.cmake
CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
Assert failed: Expected 'debug;foo;debug;baz;debug;bar', got
'debug;foo;baz;debug;bar'
Call Stack (most recent call first):
ament_cmake_libraries/test/test_deduplicate.cmake:21 (assert_equal)
CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
Assert failed: Expected 'debug;foo;debug;bar;debug;baz;bar', got
'debug;foo;debug;baz;bar'
Call Stack (most recent call first):
ament_cmake_libraries/test/test_deduplicate.cmake:26 (assert_equal)
CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
Assert failed: Expected 'debug;foo;debug;bar;debug;baz;release;bar', got
'debug;foo;debug;baz;release;bar'
Call Stack (most recent call first):
ament_cmake_libraries/test/test_deduplicate.cmake:31 (assert_equal) After: no error |
see ament#448 (comment) Signed-off-by: Vincent Richard <[email protected]>
@cottsay by the way, in your test you use the "release" keyword, but I don't think it is used in the original code |
Well spotted! Please feel free to change that to one of the supported keywords. |
@cottsay maybe it make more sense to have this in a different PR, as this one focuses on performance only. |
Sure, #516 |
Signed-off-by: Scott K Logan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
Signed-off-by: Vincent Richard <[email protected]> Signed-off-by: Scott K Logan <[email protected]> Co-authored-by: Scott K Logan <[email protected]>
A faster implementation of
ament_libraries_deduplicate()
.See #447
Also related: #442