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

Disable loading scalars as addresses when materializing an entity variable #1452

Merged

Conversation

augusto2112
Copy link

This PR disables the incorrect loading of scalars as addresses in the context of the entity variable's materializer, which caused problems when materializing a variable from a register. This also enables a test which was affected by this bug on Linux.

@adrian-prantl @vedantk although this bug manifested on Linux, I believe it could affect other platforms as well, right? Also, the failing test happened to catch this behavior, but it wasn't really its intended purpose. Do you think it'd be worthwhile to craft a new test to check this behavior specifically?

@adrian-prantl adrian-prantl self-requested a review July 14, 2020 17:59
Copy link
Member

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Fixing bugs by removing dubious workarounds is the best.

@adrian-prantl
Copy link
Member

@swift-ci test

@adrian-prantl
Copy link
Member

@augusto2112 Can you create the same pull request for master-next? And can you also make one for swift/release/5.3? I'd like to at least discuss the option of taking this for the 5.3 branch.

@adrian-prantl
Copy link
Member

@augusto2112 It would be great if you could add the test you showed me on IRC that explicitly showcases this. It was very simple, and we should have it in the test suite.

@augusto2112 augusto2112 force-pushed the class-constrained-protocol-eval branch from d97db90 to fdac8f9 Compare July 14, 2020 18:41
@augusto2112
Copy link
Author

@adrian-prantl could you take a look and see if the test is all right?

@adrian-prantl
Copy link
Member

@swift-ci test

@augusto2112
Copy link
Author

@adrian-prantl It looks like the Linux test timed out: Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..Build timed out (after 83 minutes). Marking the build as failed. Do we re-run them?

@adrian-prantl
Copy link
Member

@swift-ci test

@adrian-prantl
Copy link
Member

If it happens again I'd be more suspicious.

@augusto2112 augusto2112 force-pushed the class-constrained-protocol-eval branch 2 times, most recently from b8ee781 to 4599655 Compare July 15, 2020 17:50
@augusto2112
Copy link
Author

@adrian-prantl I thought we merged this 😮

@augusto2112 augusto2112 force-pushed the class-constrained-protocol-eval branch from 4599655 to 45b9f0d Compare August 11, 2020 23:58
@augusto2112 augusto2112 force-pushed the class-constrained-protocol-eval branch from 45b9f0d to b1cb186 Compare August 11, 2020 23:59
@augusto2112
Copy link
Author

@adrian-prantl Looks like the changes got to master in the 5.3 version. All that's left here is the removal of the obsolete m_is_generic variable.

@adrian-prantl
Copy link
Member

@swift-ci test and merge

@adrian-prantl
Copy link
Member

Can you do master-rebranch and master-next, too?

@adrian-prantl
Copy link
Member

@swift-ci test

@augusto2112
Copy link
Author

master-next was already merged: #1453
master-rebranch: #1637

@adrian-prantl adrian-prantl merged commit bc068a6 into apple:swift/master Aug 12, 2020
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