Mike's code review #2
Conversation
| use crate::errors::{self, FnPointerCannotBeAsync, FnPointerCannotBeConst, MacroExpandsToAdtField}; | ||
| use crate::{exp, fluent_generated as fluent}; | ||
|
|
||
| /// MDE: the prefix for what? For a file name? For program points? |
There was a problem hiding this comment.
For output files. E.g., if the crate name is alacritty, then that will be the OUTPUT_PREFIX, and output files will be named alacritty.decls, alacritty.dtrace, etc.
Or if compilation was invoked on the crate root file named alacritty.rs, then the behavior will match the above.
| pub parser: &'a Parser<'a>, | ||
|
|
||
| // For appending impl blocks to the file | ||
| /// MDE: Please explain. |
There was a problem hiding this comment.
When we walk a file, we process every struct definition struct X { ... } defined in the file. Upon seeing a struct definition, we must define methods like X::dtrace_print_fields. Then whenever a parameter has type X, such as fn foo(x: X) { ... }, we can call
fn foo(x: X) {
x.dtrace_print_fields(...);
}So, we synthesize an impl block, such as
impl X {
pub fn dtrace_print_fields(&self, ...) { ... }
}When we synthesize this impl block, we add it to the queue of impl blocks and other things that need to be appended to the end of the file. All these things go in this mod_items field, as in "items to append to this module".
As of right now, I don't think anything else goes into this queue, so I could use a more specific name like "impl_queue".
|
|
||
| // Given a reduced type String (i.e., no references or junk in front), | ||
| // check if the type String is a primitive (i32, u32, String, etc.) | ||
| /// MDE: What is the definition of "reduced"? Is that a standard Rust |
There was a problem hiding this comment.
In Rust the primitive types are just i32, bool, etc., but some compound types are &i32 or &'a mut bool, so a "reduced type" indicates that that ty_str contains one of the former type names in a String.
Update: reduced here means the same as Basic above.
| BasicType::Error | ||
| } | ||
|
|
||
| /// MDE: What is an "argument path"? Is it related to arguments or |
There was a problem hiding this comment.
rustc represents all types via a Path struct, since types can be long and delimited by ::, such as a::b::c::d. By argument path, I mean the argument type represented as a Path struct. So if there is a Vec<T>, the argument type would be T, so the Path struct would represent the type T.
| /// MDE: What is an "argument path"? Is it related to arguments or | ||
| /// parameters or something else? | ||
| // Given argument path representing the element type of a Vec, | ||
| /// MDE: What does "complete" mean here? |
There was a problem hiding this comment.
Complete means that we will return a BasicType that indicates the type of the Vec, rather than just the argument type, i.e., T in Vec<T>. It would be sufficient to just say "returns the BasicType of the Vec."
| // Represents a Rust type. | ||
| /// MDE: I don't understand how the following sentence belongs here in the | ||
| /// struct definition. Nor do I understand what "handled" means. | ||
| // Information about references is handled in get_basic_type. |
There was a problem hiding this comment.
I have often heard people use the phrase "reference-type", and I distinguish between a variable's type depending on whether it is some reference or not. This is just making clear that this enum does not encode any information about references, and that is stored in a separate variable (is_ref) when this enum is used.
| // crate. | ||
| #[derive(PartialEq)] | ||
| /// MDE: What is "basic" about this type? Could you name it RustType or the like? | ||
| enum BasicType { |
There was a problem hiding this comment.
See previous comment. Basic would just imply that there is no information encoded about references or pointers.
| UserDefVec(String), | ||
| PrimArray(String), | ||
| UserDefArray(String), | ||
| /// MDE: Is this only ever used for void-returning functions? |
There was a problem hiding this comment.
When asked a question in a review, please don't answer in the pull request.
Instead, improve the code so that the question does not arise in the future.
Thanks!
|
@benwu25 A ping to address the comments in this code review by editing the underlying code. Then please assign it back to me. |
This resulted in the following error for me:
error[E0464]: multiple candidates for `rlib` dependency `rustc_literal_escaper` found
--> clippy_dev/src/lib.rs:25:1
|
25 | extern crate rustc_literal_escaper;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: candidate #1: /home/pkrones/.rustup/toolchains/nightly-2025-12-25-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_literal_escaper-4f30574e01c3dad1.rlib
= note: candidate #2: /home/pkrones/.rustup/toolchains/nightly-2025-12-25-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_literal_escaper-c8a0e29ad1040cee.rmeta
Other tools in the Rust repo also take that as an explicit dependency, like rust-analyzer or lint-docs.
No description provided.