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

Remove Type.size #11

Closed
wants to merge 1 commit into from
Closed

Conversation

YerinAlexey
Copy link
Contributor

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
  • The "Unreleased" section in the changelog has been updated, if applicable

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.
@garritfra
Copy link
Owner

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

@YerinAlexey
Copy link
Contributor Author

YerinAlexey commented Jun 16, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

That should also work

@garritfra
Copy link
Owner

garritfra commented Jun 17, 2022

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
Copy link
Contributor Author

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

@garritfra
Copy link
Owner

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
Copy link
Contributor Author

Moved to #12

@YerinAlexey YerinAlexey deleted the remove-size branch June 17, 2022 18:35
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.

None yet

2 participants