-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Document all public items in rustc_incremental
#90407
Conversation
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
Thank you, @r00ster91! I forgot the recommend style. |
3e02dc8
to
3ee4b6b
Compare
pub fn staging_dep_graph_path(sess: &Session) -> PathBuf { | ||
in_incr_comp_dir_sess(sess, STAGING_DEP_GRAPH_FILENAME) | ||
} | ||
/// Returns the path to the dependency graph directory. | ||
pub fn dep_graph_path_from(incr_comp_session_dir: &Path) -> PathBuf { | ||
in_incr_comp_dir(incr_comp_session_dir, DEP_GRAPH_FILENAME) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between the return values of these functions? Can you expand on this in the doc comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed references to "the current session". The signature tells the story, here, it seems.
How do these function docs look to you, @cjgillot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand a bit on what those path represent? I mean:
- what are stored in all these files?
- the difference between dep-graph and staging dep-graph?
- what are work products?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to describe the query cache here.
@@ -664,6 +676,7 @@ fn is_old_enough_to_be_collected(timestamp: SystemTime) -> bool { | |||
timestamp < SystemTime::now() - Duration::from_secs(10) | |||
} | |||
|
|||
/// Runs garbage collection for the current session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It garbage collects all incremental session directories except the current session, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I'll double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The garbage collection logic is complex enough here that I wouldn't feel comfortable discussing its behavior in the public doc comment without input from someone familiar with this code. (Although I suspect you are right!)
cac82a2
to
cc9c544
Compare
d2f3a3a
to
1eea8ac
Compare
pub fn staging_dep_graph_path(sess: &Session) -> PathBuf { | ||
in_incr_comp_dir_sess(sess, STAGING_DEP_GRAPH_FILENAME) | ||
} | ||
/// Returns the path to the dependency graph directory. | ||
pub fn dep_graph_path_from(incr_comp_session_dir: &Path) -> PathBuf { | ||
in_incr_comp_dir(incr_comp_session_dir, DEP_GRAPH_FILENAME) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand a bit on what those path represent? I mean:
- what are stored in all these files?
- the difference between dep-graph and staging dep-graph?
- what are work products?
pub fn staging_dep_graph_path(sess: &Session) -> PathBuf { | ||
in_incr_comp_dir_sess(sess, STAGING_DEP_GRAPH_FILENAME) | ||
} | ||
/// Returns the path to the dependency graph for a given session directory. | ||
pub fn dep_graph_path_from(incr_comp_session_dir: &Path) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only used once, with the current session's directory. Is there a reason to keep it, or should it be replaced by dep_graph_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's inlined, DEP_GRAPH_FILENAME
has to be made public. I think it makes sense to have all these in one place, even if the function is only used once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean inlining it. I mean using dep_graph_path instead.
@@ -40,6 +43,7 @@ pub fn copy_cgu_workproduct_to_incr_comp_cache_dir( | |||
Some((work_product_id, work_product)) | |||
} | |||
|
|||
/// Removes files for a given work product. | |||
pub fn delete_workproduct_files(sess: &Session, work_product: &WorkProduct) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is never used outside the crate.
☔ The latest upstream changes (presumably #90731) made this pull request unmergeable. Please resolve the merge conflicts. |
3941f5f
to
a358d3b
Compare
☔ The latest upstream changes (presumably #91019) made this pull request unmergeable. Please resolve the merge conflicts. |
r=me after rebase. Thanks @pierwill ! |
3602c51
to
fa054cf
Compare
@bors r+ rollup |
📌 Commit fa054cf has been approved by |
…s, r=cjgillot Document all public items in `rustc_incremental` Also: - Review and edit current docs - Enforce documentation for the module.
@bors r- |
Looking at this now. |
Also: - Review and edit current docs - Enforce documentation for crate Co-authored-by: r00ster <[email protected]> Co-authored-by: Camille Gillot <[email protected]>
fa054cf
to
41f7692
Compare
This should be ready now. @matthiaskrgr |
@rustbot ready |
@bors r+ rollup |
📌 Commit 41f7692 has been approved by |
…s, r=cjgillot Document all public items in `rustc_incremental` Also: - Review and edit current docs - Enforce documentation for the module.
…s, r=cjgillot Document all public items in `rustc_incremental` Also: - Review and edit current docs - Enforce documentation for the module.
…s, r=cjgillot Document all public items in `rustc_incremental` Also: - Review and edit current docs - Enforce documentation for the module.
…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#90407 (Document all public items in `rustc_incremental`) - rust-lang#90897 (Fix incorrect stability attributes) - rust-lang#91105 (Fix method name reference in stream documentation) - rust-lang#91325 (adjust const_eval_select documentation) - rust-lang#91470 (code-cov: generate dead functions with private/default linkage) - rust-lang#91482 (Update documentation to use `from()` to initialize `HashMap`s and `BTreeMap`s) - rust-lang#91524 (Fix Vec::extend_from_slice docs) - rust-lang#91575 (Fix ICE on format string of macro with secondary-label) - rust-lang#91625 (Remove redundant [..]s) - rust-lang#91646 (Fix documentation for `core::ready::Ready`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Also: