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

Use std::string_view instead of string when applicable to avoid heap allocations #683

Open
r4stered opened this issue Apr 18, 2024 · 2 comments
Labels
enhancement New feature or request PathPlannerLib Changes to PathPlannerLib

Comments

@r4stered
Copy link
Contributor

For example, in NamedCommands, we pass an std::string, instead of a std::string_view. Passing a string_view would save some allocations.

@r4stered r4stered added the enhancement New feature or request label Apr 18, 2024
@mjansen4857 mjansen4857 added the PathPlannerLib Changes to PathPlannerLib label Sep 13, 2024
@mjansen4857 mjansen4857 added this to the 2025 milestone Sep 13, 2024
@mjansen4857
Copy link
Owner

So I looked into this a while back but I'm not sure I trust my own C++ knowledge enough to know where is safe to do so without the potential of problems. Especially with stuff like named commands where wpilib includes a map implementation that uses a string_view as a key. The actual string has to live somewhere and I'm not sure the what the best way to implement that is.

My approach at this point would probably just be to use string references for accessing maps and such, which should also avoid some allocations. But if you know what you're doing with respect to this string_view stuff, feel free to open a PR for it and I'll go for that over what I would do.

@r4stered
Copy link
Contributor Author

r4stered commented Oct 2, 2024

At least in the case of NamedCommands, I've only ever seen them used with string literals, which will be around for the whole robot program. So creating a string_view from a literal is fine.

I am helping out the Choreo and PhotonVision team with their C++ vendorlib stuff currently, so if I have time I can take a look and make a PR :)

@mjansen4857 mjansen4857 modified the milestones: 2025 Beta 1, 2025 Beta 2 Oct 2, 2024
@mjansen4857 mjansen4857 modified the milestones: 2025 Beta 2, 2025 Beta 3 Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PathPlannerLib Changes to PathPlannerLib
Projects
None yet
Development

No branches or pull requests

2 participants