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

Make FileId non-zero #144

Merged
merged 2 commits into from
Jan 5, 2020
Merged

Make FileId non-zero #144

merged 2 commits into from
Jan 5, 2020

Conversation

etaoins
Copy link
Contributor

@etaoins etaoins commented Jan 4, 2020

When porting from codespan 0.3 Arret I needed to track the FileId separately from the Span now that the file isn't implied in the span itself. Having a simple struct like this works for 90% of the cases:

struct Span {
  file_id: codespan::FileId,
  codespan_span: codespan::Span,
}

However, this requires that every time we parse any source we require a FileId which requires a codespan::Files to create. This is overkill in a couple of cases:

  1. Unit tests that just need to parse simple Arret strings as a shorthand for building our AST

  2. Syntax highlighting in the REPL

The compromise in etaoins/arret#160 is to use Option<FileId>. I'm not 100% happy with that as the ultimate solution but it was the easiest path to take during porting. However, this makes the size of the above struct 13 bytes which will typically be rounded up to 16 bytes for alignment and padding.

We can use std::num::NonZeroU32 instead and offset all of our indexes by 1. This allows the Rust compiler to use 0 to represent the None case and bring the size down to 12 bytes. Because the struct only needs an alignment of 4 bytes this is more likely to stay as 12 bytes in most contexts.

A nicer solution would be if Rust had e.g. NonMaxU32 but it appears the Rust team instead wants to use const generics so arbitrary niche values can be specified.

When porting from `codespan` 0.3 Arret I needed to track the `FileId`
separately from the `Span` now that the file isn't implied in the span
itself. Having a simple struct like this works for 90% of the cases:

```rust
struct Span {
  file_id: codespan::FileId,
  codespan_span: codespan::Span,
}
```

However, this requires that every time we parse any source we require a
`FileId` which requires a `codespan::Files` to create. This is overkill
in a couple of cases:

1. Unit tests that just need to parse simple Arret strings as a
   shorthand for building our AST

2. Syntax highlighting in the REPL

The compromise in etaoins/arret#160 is to use `Option<FileId>`. I'm not
100% happy with that as the ultimate solution but it was the easiest
path to take during porting. However, this makes the size of the above
struct 13 bytes which will typically be rounded up to 16 bytes for
alignment and padding.

We can use `std::num::NonZeroU32` instead and offset all of our indexes
by 1. This allows the Rust compiler to use 0 to represent the `None`
case and bring the size down to 12 bytes. Because the struct only needs
an alignment of 4 bytes this is more likely to stay as 12 bytes in most
contexts.

A nicer solution would be if Rust had e.g. `NonMaxU32` but it appears
the Rust team instead wants to use const generics so arbitrary niche
values can be specified.
We can't derive this automatically as `HeapSizeOf` doesn't know about
`NonZeroU32`
@brendanzab
Copy link
Owner

Awesome, I've been thinking about this too. This might make #138 easier too. 🤔

@brendanzab brendanzab merged commit c760bff into brendanzab:master Jan 5, 2020
@etaoins
Copy link
Contributor Author

etaoins commented Jan 5, 2020

@brendanzab

I think so. From what I understand the compiler heuristics are pretty clever. Having a FileId anywhere in a struct should make an Option of that struct "free" from a memory layout perspective.

brendanzab added a commit that referenced this pull request Jan 6, 2020
This removes the implicit conversions for the `Files::{add, update}` methods now that we parameterise `Files` by the source type (see #144).
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