deepsize icon indicating copy to clipboard operation
deepsize copied to clipboard

Clarify internal methods

Open dtolnay opened this issue 6 years ago • 2 comments

Currently two methods of the DeepSizeOf trait are documented as being "internal".

https://github.com/Aeledfyr/deepsize/blob/8c872164c27927ab06a74bf7cabd639fbe118f6d/src/lib.rs#L80-L82

https://github.com/Aeledfyr/deepsize/blob/8c872164c27927ab06a74bf7cabd639fbe118f6d/src/lib.rs#L92-L94

I don't recognize "internal" as being standard Rust terminology. If these methods are intended to be private implementation details of the deepsize crate, implemented and called only from deepsize code, then they should not be rendered in public API documentation.

@Aeledfyr

dtolnay avatar Feb 17 '19 21:02 dtolnay

I agree that they are implementation details, but they are required to be able to implement DeepSizeOf on a struct that uses heap allocation. The problem is that these methods need mutable state to be able to track references, and that has to be passed somehow. These should only be called from within an implementation of the deep_size_of_children function, but they can't be hidden or private because they are needed to be able to implement the trait. I'm not sure what the best solution to this would be. I might be able to combine or remove one of recurse_deep_size_of and deep_size_of_children to make it more consistent, so that there would only be one implementation method.

Aeledfyr avatar Mar 31 '19 14:03 Aeledfyr

I think clarifying what "internal" means (as you did in your response) would be sufficient. It seems likely that a user would read that something is "internal" and understand that to mean they are not supposed to implement it themselves.

dtolnay avatar Mar 31 '19 17:03 dtolnay