Skip to content

Conversation

@QIU1995NONAME
Copy link
Contributor

@QIU1995NONAME QIU1995NONAME commented Sep 22, 2025

  • I agree to follow the project's code of conduct.
  • I added an entry to rstar/CHANGELOG.md if knowledge of this change could be valuable to users.

@michaelkirk
Copy link
Member

This seems reasonable. Should we include implementations for other smart pointer types (at least Rc)?

@QIU1995NONAME
Copy link
Contributor Author

Hi, I originally wanted to implement something like this:

impl<P, T> RTreeObject for P
where
    P: Deref<Target = T> + Drop,
    T: RTreeObject + ?Sized,

or

impl<P, T> RTreeObject for P
where
    P: Borrow<T> + Drop,
    T: RTreeObject + ?Sized,

However, it conflicts with the existing implementations.
If I just copy the existing impl<T> RTreeObject for Arc<T> block, it ends up duplicating quite a bit of code.

Do you have any suggestions on how to handle this more cleanly?

Thanks!

@adamreichold
Copy link
Member

However, it conflicts with the existing implementations.

What is the conflict? Can the conflicting implementation be subsumed under a more general one?

@QIU1995NONAME
Copy link
Contributor Author

Right now, the known conflicts include things like:

pub struct CachedEnvelope<T: RTreeObject> 

and

impl<P> RTreeObject for P
where
    P: Point,

These overlap with the more generic impl<P, T> patterns I mentioned earlier.

@QIU1995NONAME QIU1995NONAME changed the title Update rtree object: impl RTreeObject and PointDistance for Arc<T> Update rtree object: impl RTreeObject for Arc and Rc Nov 4, 2025
Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Arc/Rc impls look good to me.

re: adding Cargo.lock - personally I'm in favor of checking in Cargo.lock, but it could generate some discussion, so let's put that in a separate commit and PR.

@QIU1995NONAME
Copy link
Contributor Author

QIU1995NONAME commented Nov 6, 2025

re: adding Cargo.lock - personally I'm in favor of checking in Cargo.lock, but it could generate some discussion, so let's put that in a separate commit and PR.

As you can see, ignoring the Cargo.lock file causes the test case (georust/geo-ci:rust-1.63) to fail.

Should we update the CI test case to handle this, or would it be better to commit the Cargo.lock file first?

Note: This issue has already been resolved in PR #213, where the Cargo.lock file is included.

@michaelkirk michaelkirk mentioned this pull request Nov 6, 2025
2 tasks
@QIU1995NONAME QIU1995NONAME force-pushed the update-rtree-object branch 2 times, most recently from 13c0ba9 to 7069e12 Compare November 10, 2025 02:34
@michaelkirk
Copy link
Member

Can you please update the changelog and remove your changes to .gitignore

@michaelkirk michaelkirk added this pull request to the merge queue Nov 11, 2025
Merged via the queue into georust:master with commit 5363825 Nov 11, 2025
6 checks passed
@QIU1995NONAME QIU1995NONAME deleted the update-rtree-object branch November 12, 2025 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants