Remove Type.size - Githubissues

garritfra / qbe-rs

QBE IR in natural Rust data structures

https://c9x.me/compile

Other

80 stars 7 forks source link

Remove Type.size #11

Closed YerinAlexey closed 2 years ago

YerinAlexey commented 2 years ago

Description

It doesn't produce correct results for aggregates and needs a reference to the module if it wants to do so. I think it might be better to implement size measurement in the frontends instead.

ToDo

[ ] Proposed feature/fix is sufficiently tested
[ ] Proposed feature/fix is sufficiently documented
[x] The "Unreleased" section in the changelog has been updated, if applicable

garritfra commented 2 years ago

Undecided. I think it's better to point this out via documentation instead of removing it entirely.

YerinAlexey commented 2 years ago

Dunno, QBE types on their own (e.g. aggregate definition is needed to infer its type) don't have a size and this function was a result of flawed compiler design

garritfra commented 2 years ago

What about this?

https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute

According to the crates.io stats this crate is actually used by people, so I'm a bit hesitant to just rip out a feature without notice.

YerinAlexey commented 2 years ago

That should also work

garritfra commented 2 years ago

Thinking about it, I think the root issue lies in Type::Aggregate(String). Shouldn't I be able to enclose a reference to a typedef instead of the name as a string? For example, I feel like this snippet is more complicated than it should be.

Fixing this would make it possible to compute the size of aggregates.

YerinAlexey commented 2 years ago

Seems reasonable, but I'm not sure how ownership will work in this case

garritfra commented 2 years ago

We'll have to introduce lifetimes for that, but I'm not yet sure how that would look. Do you feel like taking this on, or should I open an issue for later reference?

YerinAlexey commented 2 years ago

Moved to #12