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

[wpimath] Make controllers and some trajectory classes constexpr #7343

Conversation

calcmogul
Copy link
Member

@calcmogul calcmogul commented Nov 5, 2024

No description provided.

@calcmogul calcmogul requested review from PeterJohnson and a team as code owners November 5, 2024 04:05
@calcmogul calcmogul force-pushed the wpimath-make-controllers-and-some-trajectory-classes-constexpr branch 5 times, most recently from 9410c23 to 7f2837a Compare November 5, 2024 16:57
Copy link
Member

@PeterJohnson PeterJohnson left a comment

Choose a reason for hiding this comment

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

This moves a lot of functions to the headers without making them constexpr--what's the reason for this?

@@ -66,7 +109,14 @@ class WPILIB_DLLEXPORT Trajectory {
*
* @throws std::invalid_argument if the vector of states is empty.
*/
explicit Trajectory(const std::vector<State>& states);
explicit Trajectory(const std::vector<State>& states) : m_states(states) {
Copy link
Member

Choose a reason for hiding this comment

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

These were moved to the header but are not constexpr?

Copy link
Member Author

@calcmogul calcmogul Nov 5, 2024

Choose a reason for hiding this comment

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

They originally were, but we need GCC 12 for constexpr std::vector, which caused CI to fail. Bumping our RPi OS version to bookworm would fix that.

In any case, the functions are short enough to be inlined.

@PeterJohnson
Copy link
Member

Does this have a measurable effect on build times?

@calcmogul calcmogul force-pushed the wpimath-make-controllers-and-some-trajectory-classes-constexpr branch from 7f2837a to 99bd370 Compare November 6, 2024 02:44
@calcmogul calcmogul force-pushed the wpimath-make-controllers-and-some-trajectory-classes-constexpr branch from 99bd370 to e3a68eb Compare November 6, 2024 02:49
@calcmogul
Copy link
Member Author

calcmogul commented Nov 6, 2024

Seems negligible. I just did a clean build with ccache cleared.

main:

[tav@myriad allwpilib]$ time cmake --build build-cmake-release/ -- -k 0
real    8m25.931s
user    123m56.946s
sys     5m40.639

this PR:

[tav@myriad allwpilib]$ time cmake --build build-cmake-release/ -- -k 0
real    8m14.499s
user    121m56.864s
sys     5m35.327s

(I have to skip failed compilations cuz cscore can't compile against my machine's version of OpenCV Java due to class file incompatibility.)

@PeterJohnson PeterJohnson merged commit a66fa33 into wpilibsuite:main Nov 7, 2024
41 checks passed
@calcmogul calcmogul deleted the wpimath-make-controllers-and-some-trajectory-classes-constexpr branch November 7, 2024 21:33
sciencewhiz added a commit to sciencewhiz/frc-docs that referenced this pull request Nov 8, 2024
sciencewhiz added a commit to wpilibsuite/frc-docs that referenced this pull request Nov 10, 2024
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