Minimal codegen support: numbers and function paramters #2
Conversation
|
Thanks for getting this started! Is it possible to put bootstrap and CI into its own PR so that can be merged first, before the codegen part? (it will take me a little while to review this all, still don't have my usual github access) |
|
Understood, I'm working on #3 |
|
This can be rebased now that #3 merged. If it is possible to split this further for smaller review units then that would be great (e.g. AST crate first and codegen separately), but if not than this shouldn't be too bad with bootstrap out of the picture. |
|
Could you put the license files into a separate PR that we can merge right away? Guess we are currently unlicensed. Otherwise I'll try to look at this tomorrow-ish. |
There was a problem hiding this comment.
Added some initial comments, but I still have a lot more to go through.
Could you please add documentation comments? Most everything that isn't a trait implementation or a trivial function could benefit (it's a bit tough to figure out how things fit together without them).
bootstrap/src/test.rs
Outdated
| @@ -106,6 +112,19 @@ impl TestCase { | |||
| log::debug!("running {:?}", command); | |||
| command.status().unwrap(); | |||
| } | |||
|
|
|||
| pub fn build_lib(&self, manifest: &Manifest) { | |||
| std::fs::create_dir_all(self.output.parent().unwrap()).unwrap(); | |||
There was a problem hiding this comment.
let output_dir = self.output.parent().unwrap() since it is used twice. But why do you need to navigate to the parent directory for this?
There was a problem hiding this comment.
@Wybxc could you clarify why this needs to change to the parent directory of output? That doesn't seem to make sense.
example/mini_core.rs
Outdated
| @@ -0,0 +1,130 @@ | |||
| #![feature(no_core, lang_items, rustc_attrs, intrinsics, decl_macro)] | |||
There was a problem hiding this comment.
This file has a lot of extra comments that should be removed before merge. Also should get a //! comment saying what its purpose is, even though it's semi-obvious.
There was a problem hiding this comment.
tests/auxiliary/ would probably be a better place for this file, since it's not really an example. And this would mirror rust-lang/rust#130693.
| #[derive(Clone, Copy)] | ||
| pub struct ModuleCtxt<'mx>(pub &'mx ModuleArena<'mx>); |
There was a problem hiding this comment.
Nit: ctx is usually the abbreviation we use in the compiler (rather than ctxt)
| pub type CDecl<'mx> = &'mx CDeclKind<'mx>; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub enum CDeclKind<'mx> { | ||
| // Typedef { name: String, ty: CType }, | ||
| // Record { name: String, fields: Vec<CDecl> }, | ||
| // Field { name: String, ty: CType }, | ||
| // Enum { name: String, values: Vec<CEnumConstant> }, | ||
| Var { name: CValue, ty: CTy<'mx>, init: Option<CExpr<'mx>> }, | ||
| } |
There was a problem hiding this comment.
Any sort of type like this that has a C representation should get a small example of produced code in the doc comment (i.e. for each enum variant here).
| } | ||
| } | ||
|
|
||
| impl Printer { |
There was a problem hiding this comment.
I think this logic should be reversed. That is, currently the whole thing looks like:
impl Printer {
fn print_decl(&mut self, ...) { /* ... */ }
fn print_other_thing(&mut self, ...) { /* ... */ }
// ...
}But this should be moved to a trait:
// Renamed from `Printer`
struct PrintCtx { pp: /* ... */ }
trait Print {
fn print(&self, ptx: &mut PrintCtx);
}And then that gets implemented for CDecl and all the other types.
| @@ -0,0 +1,131 @@ | |||
| //! Pretty printing | |||
|
|
|||
| // TODO: pretty printing test | |||
There was a problem hiding this comment.
In the interest of time it is okay to not do the most fine-grained testing, but something is needed here. I would suggest you just add some simple output tests; in each module you can have a couple small tests that just construct an AST and print it to a string, then compare against a saved file. If RUSTC_BLESS is set, overwrite the file.
|
|
| @@ -0,0 +1,18 @@ | |||
| // CHECK: filename | |||
|
|
|||
There was a problem hiding this comment.
In addition to the filecheck tests, you should add a couple of blessed tests. Add tests/codegen-output/, teach bootstrap to codegen all the .rs files and assert their output matches a .c of the same name in the same directory. And then add a --bless argument to bootstrap that will update the .c file.
Don't use that for complex tests where the MIR output is likely to change. But this would be good for simple tests like this one, and also is nice to show what the expected output is.
There was a problem hiding this comment.
Also - these tests should get a comment about what they are intended to test. I'm not sure what this one is supposed to do.
| @@ -0,0 +1,18 @@ | |||
| // CHECK: filename | |||
|
|
|||
There was a problem hiding this comment.
Also - these tests should get a comment about what they are intended to test. I'm not sure what this one is supposed to do.
| @@ -1,5 +0,0 @@ | |||
| use rustc_middle::ty::TyCtxt; | |||
There was a problem hiding this comment.
This file gets emptied, any reason to keep it?
| @@ -0,0 +1,5 @@ | |||
| /** cast from unsigned to signed | |||
There was a problem hiding this comment.
Could you add a file-level doc comment explaining what helper.h is?
This is a minimal yet complete C code generation implementation, capable of compiling functions with number-typed parameters and
asexpressions.The structure is organized as follows:
crates/rustc_codegen_c: Implementation of the codegen backend.crates/rustc_codegen_c_ast: C syntax structure for generating C source code.bootstrap: Build system for executingrustcwith this codegen backend enabled.example: Example files demonstrating the capabilities of the codegen backend.tests: Test cases for validating functionality.