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

Fix: Display Bool's size as 1 byte in crystal tool hierarchy, not 0 #13588

Merged

Conversation

HertzDevil
Copy link
Contributor

This is a more general fix than #8273 that works for any LLVM integer type whose bit width is not a multiple of 8 (although Crystal only uses i1).

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools labels Jun 21, 2023
@HertzDevil HertzDevil changed the title Display Bool's size as 1 byte in crystal tool hierarchy Display Bool's size as 1 byte in crystal tool hierarchy, not 0 Jun 21, 2023
@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 21, 2023
@straight-shoota
Copy link
Member

@HertzDevil There are some CI failures which seem to originate from this change.

@straight-shoota straight-shoota removed this from the 1.9.0 milestone Jun 23, 2023
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jun 26, 2023

Apparently this is breaking the interpreter because Crystal::FileModules previously had size 0 and now have size 1. The i1 comes here because FileModule < NonGenericModuleType:

private def create_llvm_type(type : NonGenericModuleType | GenericClassType, wants_size)
# This can only be reached if the module or generic class don't have implementors
@llvm_context.int1
end

This was ultimately introduced in 08fa6cd. If a module type truly has no includers then it is effectively NoReturn, which corresponds to @llvm_context.void. This is probably the correct interpretation but I am not 100% sure

EDIT: this broke codegen specs

@HertzDevil
Copy link
Contributor Author

We are doing the right thing here. The file-private scope type should never have been passed around and some checks in the interpreter are most likely inconsistent with normal codegen

@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 26, 2023
@straight-shoota straight-shoota changed the title Display Bool's size as 1 byte in crystal tool hierarchy, not 0 Fix: Display Bool's size as 1 byte in crystal tool hierarchy, not 0 Jun 27, 2023
@straight-shoota straight-shoota merged commit 9565027 into crystal-lang:master Jun 27, 2023
@HertzDevil HertzDevil deleted the bug/hierarchy-bool-size branch June 28, 2023 04:42
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants