Skip to content

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 11, 2017

OK, so, long ago I committed to the idea of trying to write some high-level documentation for rustc. This has proved to be much harder for me to get done than I thought it would! This PR is far from as complete as I had hoped, but I wanted to open it so that people can give me feedback on the conventions that it establishes. If this seems like a good way forward, we can land it and I will open an issue with a good check-list of things to write (and try to take down some of them myself).

Here are the conventions I established on which I would like feedback.

Use README.md files. First off, I'm aiming to keep most of the high-level docs in README.md files, rather than entries on forge. My thought is that such files are (a) more discoverable than forge and (b) closer to the code, and hence can be edited in a single PR. However, since they are not in the code, they will naturally get out of date, so the intention is to focus on the highest-level details, which are least likely to bitrot. I've included a few examples of common functions and so forth, but never tried to (e.g.) exhaustively list the names of functions and so forth.
- I would like to use the tidy scripts to try and check that these do not go out of date. Future work.

librustc/README.md as the main entrypoint. This seems like the most natural place people will look first. It lays out how the crates are structured and is intended to give pointers to the main data structures of the compiler (I didn't update that yet; the existing material is terribly dated).

A glossary listing abbreviations and things. It's much harder to read code if you don't know what some obscure set of letters like infcx stands for.

Major modules each have their own README.md that documents the high-level idea. For example, I wrote some stuff about hir and ty. Both of them have many missing topics, but I think that is roughly the level of depth that would be good. The idea is to give people a "feeling" for what the code does.

What is missing primarily here is lots of content. =) Here are some things I'd like to see:

  • A description of what a QUERY is and how to define one
    • Some comments for librustc/ty/maps.rs
  • An overview of how compilation proceeds now (i.e., the hybrid demand-driven and forward model) and how we would like to see it going in the future (all demand-driven)
  • Some coverage of how incremental will work under red-green
  • An updated list of the major IRs in use of the compiler (AST, HIR, TypeckTables, MIR) and major bits of interesting code (typeck, borrowck, etc)
  • More advice on how to use x.py, or at least pointers to that
  • Good choice for config.toml
  • How to use RUST_LOG and other debugging flags (e.g., -Zverbose, -Ztreat-err-as-bug)
  • Helpful conventions for debug! statement formatting

cc @rust-lang/compiler @mgattozzi

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Wonderful! Always glad to see more docs, and these are great.

I have some small nits on formatting, take 'em or leave 'em.

Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to cut this down to one newline

Copy link
Contributor

Choose a reason for hiding this comment

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

conventionally the explicit rust here is only used in markdown files, not source files

Copy link
Contributor

Choose a reason for hiding this comment

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

Conventionally, this would be

/// # Examples

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "Rust"?

Copy link
Contributor

Choose a reason for hiding this comment

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

you may or may not want to consider using a > for this note

Copy link
Contributor

Choose a reason for hiding this comment

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

extra newlines here too

@eddyb
Copy link
Member

eddyb commented Sep 11, 2017

r? @steveklabnik

@rust-highfive rust-highfive assigned steveklabnik and unassigned eddyb Sep 11, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

change -> chance?

Copy link
Contributor

Choose a reason for hiding this comment

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

typo: find find

@mgattozzi
Copy link
Contributor

@nikomatsakis I'm glad you finally got around to this! I'll take a look at this later when I get the chance!

Copy link
Contributor

@qmx qmx Sep 12, 2017

Choose a reason for hiding this comment

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

This definition of driver as being the "main" function for the compiler is really nice. What about giving more emphasis to it on the top-level README file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unable to comment below this part of the diff, but between the analysis oasses and translation to LLVM there's also HAIR and MIR lowering. Would be nice to mention those as well to see how they fit into the big picture. (I believe HAIR is where the typeck tables are terminated (as in flushed out), for example?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think this part is basically all dated. I just pulled it unedited from before.

Copy link
Contributor

Choose a reason for hiding this comment

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

and you must X some function

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this newline serve a purpose?

Copy link
Contributor

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Definitely helpful. Left a few comments but none of them major.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've wondered what the S in TyS is for. And similarly the s in sty. Maybe this is one place to mention it.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "the typechecker" or "typechecking"?

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nothing after "reused" seems to add anything.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 13, 2017
@qmx qmx mentioned this pull request Sep 14, 2017
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 15, 2017
bring TyCtxt into scope

got comments both from @eddyb and @nikomatsakis (via rust-lang#44505) that we should always put `TyCtxt` in scope

should I just go and import it at other places in the codebase or we just keep doing small improvements?
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017
bring TyCtxt into scope

got comments both from @eddyb and @nikomatsakis (via rust-lang#44505) that we should always put `TyCtxt` in scope

should I just go and import it at other places in the codebase or we just keep doing small improvements?
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 16, 2017
bring TyCtxt into scope

got comments both from @eddyb and @nikomatsakis (via rust-lang#44505) that we should always put `TyCtxt` in scope

should I just go and import it at other places in the codebase or we just keep doing small improvements?
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 17, 2017
bring TyCtxt into scope

got comments both from @eddyb and @nikomatsakis (via rust-lang#44505) that we should always put `TyCtxt` in scope

should I just go and import it at other places in the codebase or we just keep doing small improvements?
@nikomatsakis nikomatsakis changed the title [WIP] rework the README.md for rustc and add other readmes rework the README.md for rustc and add other readmes Sep 18, 2017
@nikomatsakis
Copy link
Contributor Author

@eddyb take a look at the last two commits -- I made an attempt to document the query system (I also broke it up into multiple files, to try and hide some of the "plumbing"). I did not rename anything, because I didn't want to run around and update paths for now (though I think it would maybe be best to move this code into queries instead of ty::maps::queries).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Sep 18, 2017

📌 Commit a8a28ce has been approved by alexcrichton

@alexcrichton
Copy link
Member

@bors: p=2

@bors
Copy link
Collaborator

bors commented Sep 18, 2017

🔒 Merge conflict

@bors
Copy link
Collaborator

bors commented Sep 18, 2017

☔ The latest upstream changes (presumably #44678) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

*linked

Copy link
Contributor

Choose a reason for hiding this comment

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

better: gradually being replaced with HirId.

Copy link
Contributor

@arielb1 arielb1 Sep 19, 2017

Choose a reason for hiding this comment

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

an index identifying a definition (see librustc/hir/def_id.rs). Uniquely identifies a DefPath.

Copy link
Contributor

Choose a reason for hiding this comment

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

ICE - internal compiler error. When the compiler crashes.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Sep 19, 2017

@bors r=steveklabnik p=22

(Besides the fact that it'd be really nice to have these docs be easier to find, this PR is actually kinda' conflict prone due to breaking up the maps module.)

@bors
Copy link
Collaborator

bors commented Sep 19, 2017

📌 Commit 638958b has been approved by steveklabnik

@bors
Copy link
Collaborator

bors commented Sep 19, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Sep 19, 2017

📌 Commit 638958b has been approved by steveklabnik

@mgattozzi
Copy link
Contributor

@nikomatsakis I think everyone already covered my thoughts. I think this is a good first stab at it and addresses things I've brought up to you during Meetups so I'm hoping we can continue to do things like this and make compiler hacking more accessible! :D

@arielb1 arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2017
@bors
Copy link
Collaborator

bors commented Sep 19, 2017

⌛ Testing commit 638958b with merge f60bc3a...

bors added a commit that referenced this pull request Sep 19, 2017
rework the README.md for rustc and add other readmes

OK, so, long ago I committed to the idea of trying to write some high-level documentation for rustc. This has proved to be much harder for me to get done than I thought it would! This PR is far from as complete as I had hoped, but I wanted to open it so that people can give me feedback on the conventions that it establishes. If this seems like a good way forward, we can land it and I will open an issue with a good check-list of things to write (and try to take down some of them myself).

Here are the conventions I established on which I would like feedback.

**Use README.md files**. First off, I'm aiming to keep most of the high-level docs in `README.md` files, rather than entries on forge. My thought is that such files are (a) more discoverable than forge and (b) closer to the code, and hence can be edited in a single PR. However, since they are not *in the code*, they will naturally get out of date, so the intention is to focus on the highest-level details, which are least likely to bitrot. I've included a few examples of common functions and so forth, but never tried to (e.g.) exhaustively list the names of functions and so forth.
    - I would like to use the tidy scripts to try and check that these do not go out of date. Future work.

**librustc/README.md as the main entrypoint.** This seems like the most natural place people will look first. It lays out how the crates are structured and **is intended** to give pointers to the main data structures of the compiler (I didn't update that yet; the existing material is terribly dated).

**A glossary listing abbreviations and things.** It's much harder to read code if you don't know what some obscure set of letters like `infcx` stands for.

**Major modules each have their own README.md that documents the high-level idea.** For example, I wrote some stuff about `hir` and `ty`. Both of them have many missing topics, but I think that is roughly the level of depth that would be good. The idea is to give people a "feeling" for what the code does.

What is missing primarily here is lots of content. =) Here are some things I'd like to see:

- A description of what a QUERY is and how to define one
    - Some comments for `librustc/ty/maps.rs`
- An overview of how compilation proceeds now (i.e., the hybrid demand-driven and forward model) and how we would like to see it going in the future (all demand-driven)
- Some coverage of how incremental will work under red-green
- An updated list of the major IRs in use of the compiler (AST, HIR, TypeckTables, MIR) and major bits of interesting code (typeck, borrowck, etc)
- More advice on how to use `x.py`, or at least pointers to that
- Good choice for `config.toml`
- How to use `RUST_LOG` and other debugging flags (e.g., `-Zverbose`, `-Ztreat-err-as-bug`)
- Helpful conventions for `debug!` statement formatting

cc @rust-lang/compiler @mgattozzi
@bors
Copy link
Collaborator

bors commented Sep 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: steveklabnik
Pushing f60bc3a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.