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

Support alignof and instance_alignof #14087

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

HertzDevil
Copy link
Contributor

Resolves part of #14033 (does not include support for members within aggregates).

They are direct analogs of sizeof and instance_sizeof for the most part, so these nodes' implementations should equally be similar in the compiler. The only difference is that LLVMABIAlignmentOfType returns a LibC::UInt rather than LibC::ULongLong for LLVMABISizeOfType. On the other hand, note that LLVMAlignOf does produce a 64-bit LLVM value like LLVMSizeOf.

@straight-shoota straight-shoota added this to the 1.11.0 milestone Dec 14, 2023
@HertzDevil HertzDevil removed this from the 1.11.0 milestone Dec 14, 2023
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Dec 14, 2023

This will become invalid syntax:

def alignof(x)
end

alignof(1)

This is a breaking change no matter which Crystal version we release it. I see two ways out:

  • Make the actual keyword __alignof, then deprecate anything called alignof until 2.0. Then support alignof as a keyword while deprecating __alignof in 2.x, so that alignof eventually becomes the only supported syntax in 3.0. (MSVC did this with C++20 coroutine keywords.)
  • Do something like Compiler: include is now a macro #11868. It would make a bit more sense here because alignof is indeed an expression, rather than a statement like include. But it is also not trivial since alignof can be used inside type names as generic type arguments as well, whereas macro calls can't.

@straight-shoota
Copy link
Member

There's no feasible way to exclude minor risks of breakage when changing just about anything.
Every time we introduce a new feature with a name in a public namespace, there's a chance of breaking someones code if they were already using that name for something else (or for the same purpose but with slight differences).
I don't see a systematically difference to e.g. adding the ::EOL constant (#11303). If someone defined this constant as a convenience in their code, the definition will break now. If they were expecting a Char value, reads from the constant will also break now. Breaking is not great, but it's an acceptable risk.
alignof being a compiler primitive maybe is a bit more serious, there are a tiny bit less options to navigate around (not that there are any good ones with most stdlib features). But overall it's the same thing.

If we wanted to avoid accidentally overriding any user-defined features when adding new stuff to the compiler or stdlib, we'd need to take a lot more care and have more frequent major releases to get things out properly. Or we'd need to reserve namespaces for exclusive stdlib use. We don't have any of that and I don't think we need it.

In this case, the names are very specific and pretty clearly pointing towards something that you would expect as a compiler feature.

I think it's totally fine to proceed with this PR.

@HertzDevil HertzDevil added this to the 1.11.0 milestone Dec 14, 2023
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Dec 14, 2023

The thing about alignof is simply adding it creates syntax errors and forbids tools like the formatter and the docs generator from working, in contrast to something that breaks only during the semantic phase. So this PR has a larger surface of impact than ::EOL.

@Sija
Copy link
Contributor

Sija commented Dec 14, 2023

@HertzDevil Your commitment to BC is applaudable. In this case though I think it's an acceptable risk, given how much effort would be needed to do it "safely" and the low probability of the name clash due to the very technical term. GitHub search shows only 7 files containing that phrase and none of it seem to be problematic (in that sense).

@straight-shoota straight-shoota removed this from the 1.11.0 milestone Dec 15, 2023
@HertzDevil
Copy link
Contributor Author

Third way out: allow alignof only inside type names, and define alignof in normal code as a primitive method in the top-level namespace (or even Class#alignment). Thus AlignOf nodes can only appear inside other type nodes, the rest of them are regular Call nodes. This however means you can't write things like alignof({Int32}*) in normal code for the same reason unsafe_as cannot

@beta-ziliani
Copy link
Member

beta-ziliani commented Dec 20, 2023

I think it's best to avoid the gymnastics to avoid any breakage and let it be. As said, there aren't many occurrences of the word in the wild, so I don't think we need to be that overcautious here. If we get a report we can revert it for 1.11.1 and consider an alternative.

@straight-shoota straight-shoota added this to the 1.11.0 milestone Dec 20, 2023
@straight-shoota straight-shoota merged commit 265724d into crystal-lang:master Dec 21, 2023
55 checks passed
@HertzDevil HertzDevil deleted the feature/alignof branch December 21, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants