S2ClosestEdgeQueryBase::InitCovering: Use a single iterator #494
Conversation
benguild
changed the title
S2ClosestEdgeQueryBase…estEdgeQueryBase`
benguild
force-pushed
the
closestEdgeQueryTODO
branch
from
cc4416a to
6616076
Compare
|
Due to the complexity of this and the Go version, it'd help to have the feedback about whether or not anyone has thoughts on how to improve it, or if the TODO from the current code should just be removed from both Go and C++ otherwise. I'm happy to take another look but I can't keep this in mind forever and would need to move onto other things if there's no bandwidth to consider the changes. |
If it's going to be a problem for you to come back to the code in a few days or weeks, then it's definitely going to be a problem for someone else to look at it in a few years. Can you make sure the code is sufficiently documented so you don't need to "keep it in mind"? I will release the benchmarks soon, so you can test whether this makes a difference. Funny that it's so much more complex in go than C++. What's that? |
To clarify, I can't guarantee I won't be blocked by future events.
Go's patterns are different, but the C++ may not be totally correct as mentioned since I don't work with it often. (hence the "draft" PR and RFC) |
| // Don't need to reserve index_cells_ since it is an InlinedVector. | ||
| index_covering_.reserve(6); | ||
|
|
||
| // TODO(ericv): Use a single iterator (iter_) below and save position |
There was a problem hiding this comment.
S2ClosestEdgeQueryBase has an iter_ member, which is probably what this TODO is referring to:
s2geometry/src/s2/s2closest_edge_query_base.h
Line 452 in fd727eb
|
There are many benchmarks in s2geometry/src/s2/s2closest_edge_query_test.cc Line 1001 in f79c017 Do we have coverage for this function and does PR speed them up? |
2 participants
This is sibling to: golang/geo#248
While resolving a bug with the code sync between Go/C++ (golang/geo#247) I noticed the TODO.
Since there's seemingly an effort to mirror implementations, I'm opening a draft PR, because I barely work with C++ and I'd need someone to take a look at this.