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.
Slightly different from my original proposal. There are a few pieces worth highlighting.
First, I think the most idiomatic Rust approach would actually be to drop the
n
argument as well ass
, and let the size of thep
argument imply the order of the polynomial. The reason I didn't take this further step is that keepingn
requires fewer changes, and leaves the code a closer parallel to its C++ and Java counterparts. I figure we can always choose to reconsider later, perhaps if and when we focus on performance. The performance implications are likely minor, and I'm not sure which approach they favor.Second, while keeping
n
, it's somewhat tempting to change it tousize
, since its support for the negative special case doesn't appear to be used in any of Karney's code that I've examined or logged. It's certainly not used in any of our current geodesic functionality. It may be used in geoid, magnetic model, or projection special cases that aren't hit by any of the scenarios I've run in my instrumented C++. My current guess is that it's not actually used and was only added for completeness. Anyway, I left it signed for now, but changed it from i64 to isize since we ultimately need to cast it to usize for use as an array index, and any value too large for i32 would strongly hint that we're already off the rails. The largestn
I've seen in instrumented scenarios is 29.Third, I went with a slightly different polyval implementation than my original proposal. I felt that my new version is slightly more clear that it never tries to cast a negative
n
to usize (though both should actually be safe), and that it might give the optimizer more of a hint that we don't care about thei
value except for accessing an element ofp
(though I suspect it would recognize that anyway).Fourth, I tried to keep changes to polyval's callers minimal in most cases, but there were two I couldn't resist: Both
_C3f
and_C4f
were using floating point values to represent array indices. I switched these to usize, also tweaking the types of some variables they interact with.Finally, these changes should be behavior-neutral. I think they'll help me with arcdirect troubleshooting because polyval is called extensively in some sections of code used to calculate Geodesic member variables that end up with suspect values, and these changes will make it easier for me to compare the C++ and Rust code for these sections. However, they don't do anything to resolve the arcdirect problems themselves.
I'd be happy to revise my approach if you'd prefer something different for any of these various items.
CHANGES.md
if knowledge of this change could be valuable to users.