Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify internal methods #6

Open
dtolnay opened this issue Feb 17, 2019 · 2 comments
Open

Clarify internal methods #6

dtolnay opened this issue Feb 17, 2019 · 2 comments
Assignees

Comments

@dtolnay
Copy link

dtolnay commented Feb 17, 2019

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

deepsize/src/lib.rs

Lines 80 to 82 in 8c87216

/// This is an internal function, and requires a [`Context`](Context)
/// to track visited references.
fn recurse_deep_size_of(&self, context: &mut Context) -> usize {

deepsize/src/lib.rs

Lines 92 to 94 in 8c87216

/// This is an internal function, and requires a [`Context`](Context)
/// to track visited references.
fn deep_size_of_children(&self, context: &mut Context) -> usize;

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

@Aeledfyr
Copy link
Owner

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 Aeledfyr self-assigned this Mar 31, 2019
@dtolnay
Copy link
Author

dtolnay commented Mar 31, 2019

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.

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

No branches or pull requests

2 participants