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

Compiler: don't precompute sizeof on abstract structs and modules #7801

Merged
merged 3 commits into from May 21, 2019

Conversation

@asterite
Copy link
Member

commented May 20, 2019

Fixes #7741

And a few other fixes/improvements too.

I'm marking it as a breaking change too because instance_sizeof could be incorrectly invoked with non-class stuff (like structs) and so this will make compiling (but incorrect) code to stop compiling... but I think it's fine.

@jhass

jhass approved these changes May 20, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Some explanation: at some point we made sizeof be computed, if possible, at the semantic phase so that it can be used for generic type arguments, like StaticArray(UInt8, sizeof(Int32)). That works fine, but when the type given to sizeof is an abstract thing like a struct or a module, and there are generic subtypes, the final size (which is the size of the union of all actual instantiations in the program) can only be known at the end of compilation. This is what this PR fixes.

@@ -2568,7 +2568,8 @@ module Crystal
# Try to resolve the sizeof right now to a number literal
# (useful for sizeof inside as a generic type argument, but also
# to make it easier for LLVM to optimize things)
if type && !node.exp.is_a?(TypeOf)
if type && !node.exp.is_a?(TypeOf) &&
!(type.module? || (type.abstract? && type.struct?))

This comment has been minimized.

Copy link
@bcardiff

bcardiff May 21, 2019

Member

Can't this be moved upwards together with the check of type.is_a?(GenericType)? I thought the notion of an instantiable type was already defined somewhere.

This comment has been minimized.

Copy link
@asterite

asterite May 21, 2019

Author Member

Here in MainVisitor we compute it if we can. Later in CleanupTransformer we check that it's only a class. So, doing sizeof an abstract struct or module can be done and works, just not eagerly.

To be honest, now I'm not convinced that sizeof should work eagerly at all. I did that so one can do, for example, StaticArray(Int32, sizeof(Int32)), but that's a pretty contrived example. But for now I'm just fixing broken things, not changing the semantics of the language or introducing breaking changes. But eventually we can define the semantics we want for this and fix it.

And no, unfortunately there's no type.can_be_instantiated? method.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Can confirm this fixed my issue. Thanks again.

@asterite asterite added this to the 0.29.0 milestone May 21, 2019

@asterite asterite merged commit ee0df4e into crystal-lang:master May 21, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asterite asterite deleted the asterite:bug/sizeof branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.