Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/Fix pdp.GetAllCurrents() #6025
Add/Fix pdp.GetAllCurrents() #6025
Changes from 1 commit
697477f
74b391f
908b11b
ba2373f
914dc8a
dd2f92e
8ae665f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@PeterJohnson should we make this be a SmallVector too? This could be a pretty hot path.
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.
I choose std::vector as it was the return type of all the NetworkTables get__Array() like GetDoubleArray(). I'm neutral either way.
The API call here should be the vector/SmallVector though, as I don't see anywhere in the C++ APIs that we return arrays.
Honestly, if speed was a concern, I think in the JNI service, we could go back to my initial prototype of just a straight double[24] array. We don't need a vector there as we could just allocate to the biggest PDP size, and the HAL filling the array means it doesn't need to be initialized too. Since it doesn't leave the function, we don't need the dynamic sizing there. I just didn't see as much array support in C++ (and I'm not a native C++ developer).
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.
So typically how we do this is we have both a vector and a SmallVector overload of the same function. So you would keep the current function and add a SmallVector-taking one:
std::span<double> GetAllCurrents(wpi::SmallVectorImpl<double>& buf)
.