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.

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

Add clamp header #85

Merged
merged 2 commits into from Jul 14, 2020
Merged

Add clamp header #85

merged 2 commits into from Jul 14, 2020

Conversation

v-lopez
Copy link
Contributor

@v-lopez v-lopez commented Jul 10, 2020

Avoids temporarily avoids using boost or C++17

In the end I did not need to create another library target for this, since all templates. But I've put in the separate directory as discussed.

Fixes #84

@v-lopez v-lopez requested a review from a team as a code owner July 10, 2020 10:50
@Karsten1987
Copy link
Contributor

This looks good to me, but I guess it deserves a bit more doxygen. It would be great if you could come up with something similar to what is done in other headers, e.g.: https://github.com/ros2/rcpputils/blob/master/include/rcpputils/join.hpp#L30-L39

@Karsten1987
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor

That is my only feedback as well, to more thoroughly document the functions provided. Thanks for this contribution!

@v-lopez
Copy link
Contributor Author

v-lopez commented Jul 13, 2020

Sure, I'll do it tomorrow morning.

@emersonknapp
Copy link
Contributor

Also note that you will need to use git commit --signoff to pass the DCO check. You can do this for existing commits with git rebase --signoff -i origin/master and leaving all the commits as pick

@v-lopez v-lopez force-pushed the add-clamp branch 2 times, most recently from 209a32a to 13ddbc7 Compare July 14, 2020 06:14
Victor Lopez added 2 commits July 14, 2020 08:18
* Add clamp header to avoid using boost or C++17

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
@v-lopez
Copy link
Contributor Author

v-lopez commented Jul 14, 2020

I've addressed the documentation and signing, please take a look when you have time.

@emersonknapp
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Contributor

@emersonknapp can this be merged? Ideally we'd like to have this backported to Foxy as well to have ros_control still be compatible with Foxy.

@emersonknapp emersonknapp merged commit c730cb7 into ros2:master Jul 14, 2020
Karsten1987 pushed a commit that referenced this pull request Jul 14, 2020
* Add clamp header

* Add clamp header to avoid using boost or C++17

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

* Extend documentation of clamp methods

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Karsten1987 added a commit that referenced this pull request Jul 15, 2020
* Add clamp header

* Add clamp header to avoid using boost or C++17

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

* Extend documentation of clamp methods

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

Co-authored-by: Victor Lopez <3469405+v-lopez@users.noreply.github.com>
@wjwwood
Copy link
Member

wjwwood commented Jan 4, 2021

Uh, I just saw this in the include folder. What's the rationale for putting this in a different namespace from rcpputils?

We have a pretty consistent pattern of having the root cpp namespaces (and include namespace to match) be the same as the package name.

In my opinion, this should be moved into the rcpputils namespace.

@v-lopez
Copy link
Contributor Author

v-lopez commented Jan 4, 2021

It was done in a separate namespace after discussion in #84, in case in the future similar functions are added and this ends up on a separate package or structure.

@wjwwood
Copy link
Member

wjwwood commented Jan 4, 2021

I see, thanks for the pointer @v-lopez, but I have to really disagree with the direction. I think it should be in the rcpputils namespace until it is split into a separate package. Splitting it preemptively is just confusing to me.

I'll probably make pull requests to move it in rolling and deprecate it and provide the new location in foxy, unless someone convinces me otherwise. @emersonknapp @clalancette?

@clalancette
Copy link
Contributor

I'll probably make pull requests to move it in rolling and deprecate it and provide the new location in foxy, unless someone convinces me otherwise. @emersonknapp @clalancette?

That sounds good to me.

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.

Adding common math functions
5 participants