Skip to content

Fix Blackboard::set() for StringView #570

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

Closed
wants to merge 1 commit into from

Conversation

galou
Copy link
Contributor

@galou galou commented May 26, 2023

The second call to Blackboard::set() with the same key was not working for StringView because port_info.parseString(value) returns an empty Any.
Any(std::string(value)) is used instead for StringView when the key already exists.

@facontidavide
Copy link
Collaborator

thanks. Do you have a simple unit test in mind that reproduce the error and fails before this change is applied?

@facontidavide facontidavide self-assigned this Jun 6, 2023
@galou
Copy link
Contributor Author

galou commented Jun 6, 2023

I could try to write one. Where should I write it?

@facontidavide
Copy link
Collaborator

if you just show me a plain example, I can do it.
otherwise, add it here: https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/tests/gtest_blackboard.cpp

@galou galou force-pushed the ge/fix/set_stringview branch 3 times, most recently from 9b6f74f to 7ffe080 Compare June 7, 2023 05:26
The second call to `Blackboard::set()` with the same key was not working
for `StringView` because `port_info.parseString(value)` returns an empty
`Any`.
`Any(std::string(value))` is used instead for `StringView` when the key
already exists.

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
@galou galou force-pushed the ge/fix/set_stringview branch from 7ffe080 to e892e40 Compare June 7, 2023 05:29
@galou
Copy link
Contributor Author

galou commented Jun 7, 2023

Test case added.

facontidavide added a commit that referenced this pull request Jun 7, 2023
@facontidavide
Copy link
Collaborator

The problem was real (thanks for reporting), but your solution would have introduced a bug.

I fixed it differently here: d28e5be

@galou
Copy link
Contributor Author

galou commented Jun 7, 2023

Thanks for the fix! Just by curiosity and lack of time to study it myself, what was the introduced bug?

@galou galou deleted the ge/fix/set_stringview branch June 8, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants