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

Basic metadata generation #1820

Merged
merged 11 commits into from
Jun 15, 2022
Merged

Basic metadata generation #1820

merged 11 commits into from
Jun 15, 2022

Conversation

kennykerr
Copy link
Collaborator

I'm back working on #1093.

  • Fixed a bug in PE file generation introduced when switching to using windows-sys for the PE header structures.
  • Added basic test validation to catch such regressions.
  • Restructured some of the shared types in the metadata crate to better serve both metadata readers and writers.
  • Renamed union to explicit_layout to be faithful to ECMA-335.

@kennykerr kennykerr merged commit d3eee4a into master Jun 15, 2022
@kennykerr kennykerr deleted the writer branch June 15, 2022 21:56
self.0 |= 0x10;
}

pub fn get_abstract(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

It would be more consistent to just call this abstract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, abstract is a reserved keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's a shame. You could use it by doing r#abstract, but that's probably not worth it.

@@ -197,7 +197,7 @@ fn gen_compare_traits(gen: &Gen, def: TypeDef, name: &TokenStream, cfg: &Cfg) ->
}

fn gen_debug(gen: &Gen, def: TypeDef, ident: &TokenStream, cfg: &Cfg) -> TokenStream {
if gen.sys || gen.reader.type_def_has_union(def) || gen.reader.type_def_has_packing(def) {
if gen.sys || gen.reader.type_def_has_explicit_layout(def) || gen.reader.type_def_has_packing(def) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between gen.reader.type_def_has_explicit_layout(def) and gen.reader.type_def_flags(def).explicit_layout(). If they're equivalent, could we stick with using just one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The explicit_layout flag just checks the bit in the TypeDef record. The type_def_has_explicit_layout is a much more comprehensive inspection that checks not only the TypeDef itself, but also its fields recursively, and also any other TypeDefs of the same type but for different architectures. The latter depends on the former.

Comment on lines +28 to +34
loop {
value >>= 1;
if value == 0 {
break;
}
bits += 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
loop {
value >>= 1;
if value == 0 {
break;
}
bits += 1;
}
while { value >>= 1; value != 0 } {
bits += 1;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wow that's cool

pub fn strlen(cs: *const u8) -> usize;
}

pub fn composite_index_size(tables: &[usize]) -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this function requires a deep understanding of the metadata file format, but I do think some small comments could make this much easier to understand.

Self::ModuleRef(row) => ((row + 1) << 2) + 1,
Self::AssemblyRef(row) => ((row + 1) << 2) + 2,
Self::TypeRef(row) => ((row + 1) << 2) + 3,
_ => unimplemented!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this unimplemented or merely unexpected? Using a panic! with some sort of explanatory message would probably be better.

@@ -0,0 +1,51 @@
#[derive(Clone, Copy)]
pub enum ResolutionScope {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it would benefit from a short doc about what it's used for.

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

Successfully merging this pull request may close these issues.

2 participants