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 as s, and let the size of the p argument imply the order of the polynomial. The reason I didn't take this further step is that keeping n 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 to usize, 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 largest n 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 the i value except for accessing an element of p (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.
michaelkirk
commented
3 years ago 
bors[bot]
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
nargument as well ass, and let the size of thepargument imply the order of the polynomial. The reason I didn't take this further step is that keepingnrequires 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 largestnI'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
nto usize (though both should actually be safe), and that it might give the optimizer more of a hint that we don't care about theivalue 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
_C3fand_C4fwere 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.
[ ] I added an entry to
CHANGES.mdif knowledge of this change could be valuable to users.