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

lower array equals only if it needs code gen #14981

Closed
wants to merge 2 commits into from

Conversation

WalterBright
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 10, 2023

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
23771 enhancement Array equals should be handled by CTFE

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14981"

@WalterBright
Copy link
Member Author

Fixing this is blocked by https://issues.dlang.org/show_bug.cgi?id=23771

@RazvanN7
Copy link
Contributor

I have rebased to latest master and fixed the failing tests. It turns out we only needed to fix the failing error message.

@RazvanN7
Copy link
Contributor

This cannot be pulled unless #11212 is reverted (cc @kinke ). Prior to #11212 , before lowering, the compiler would do some type checks to make sure that the comparison may be performed (and output errors in case it was not possible). The checks were duplicated in the hook, so the PR made sense because the hook would get lowered regardless if the scope needs codegen or not. However, this PR makes it so that the lowering is performed only in codegen'ed scopes so for situations such as __traits(compiles, arr1 == arr2) you are bypassing and, implicitly, the associated type checks.

@RazvanN7
Copy link
Contributor

The 3 options we have seem to be:

@kinke
Copy link
Contributor

kinke commented May 17, 2023

I don't think the revert is a real option, unless you want to re-add __ArrayEq to druntime too, and fix the other issue that my PR apparently fixed.

Adding a EqualExp.lowering field could be one option, and ignoring it for CTFE.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Aug 3, 2023

Closing, as this is not the right approach.

@RazvanN7 RazvanN7 closed this Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants