Skip to content

Emit reabstraction thunks for implicit conversions between T.TangentType and Optional<T>.TangentType #78076

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 6, 2025
Merged

Emit reabstraction thunks for implicit conversions between T.TangentType and Optional<T>.TangentType #78076

merged 8 commits into from
Feb 6, 2025

Conversation

asl
Copy link
Contributor

@asl asl commented Dec 10, 2024 •
edited
Loading

Fixes #77871

@asl asl requested review from rxwei and hamishknight December 10, 2024 00:47
@asl
Copy link
Contributor Author

asl commented Dec 10, 2024

Tagging @JaapWijnen

@asl asl force-pushed the diff-opt-implicit-conversion branch from 49b7a36 to 5a68861 Compare December 10, 2024 00:51
@asl
Copy link
Contributor Author

asl commented Dec 10, 2024

@swift-ci Please test

@asl asl requested a review from slavapestov December 10, 2024 22:29
@asl
Copy link
Contributor Author

asl commented Dec 10, 2024

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Dec 20, 2024

@slavapestov @hamishknight ping

hamishknight
hamishknight reviewed Dec 30, 2024
View reviewed changes
@asl asl force-pushed the diff-opt-implicit-conversion branch from b47b157 to 5382e52 Compare January 18, 2025 01:02
@asl asl requested a review from eeckstein as a code owner January 18, 2025 01:02
@asl asl requested a review from hamishknight January 18, 2025 01:02
@asl
Copy link
Contributor Author

asl commented Jan 18, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 18, 2025

@hamishknight I believe I addressed all the comments. Also added test for address-only type conversion

@asl
Copy link
Contributor Author

asl commented Jan 18, 2025

@swift-ci please test

hamishknight
hamishknight approved these changes Jan 18, 2025
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM modulo a couple of nits, but nothing blocking

@asl
Copy link
Contributor Author

asl commented Jan 20, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 21, 2025

Let's check if CI is healthy on main again
@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 21, 2025

@swift-ci please test linux

@asl
Copy link
Contributor Author

asl commented Jan 21, 2025

@slavapestov Please let me know if you're having further comments here, or if this PR is good to go

@asl
Copy link
Contributor Author

asl commented Jan 24, 2025

@swift-ci please test

@asl asl force-pushed the diff-opt-implicit-conversion branch from 8b07cae to c6cc675 Compare January 31, 2025 06:03
@asl
Copy link
Contributor Author

asl commented Jan 31, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 31, 2025

@slavapestov will you please take a look when you will have a chance?

slavapestov
slavapestov approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't get notifications about your pings for some reason.

lib/AST/ASTContext.cpp
candidateModule->isStdlibModule()) {
assert(!constructorDecl && "Multiple `Optional.TangentVector.init`s");
constructorDecl = cast<ConstructorDecl>(candidate);
#ifdef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

For future PRs: we're trying to get away from conditional asserts. Instead, use ASSERT and avoid #ifdef NDEBUG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, wasn't aware of this, thanks!

@asl
Copy link
Contributor Author

asl commented Feb 5, 2025

@swift-ci please smoke test

@asl asl merged commit 1e7a1d9 into main Feb 6, 2025
5 checks passed
@asl asl deleted the diff-opt-implicit-conversion branch February 6, 2025 04:57
clackary pushed a commit to clackary/swift that referenced this pull request Feb 21, 2025
@asl @clackary
Emit reabstraction thunks for implicit conversions between T.TangentT…
8e3bdee 
…ype and Optional<T>.TangentType (swiftlang#78076)

(cherry picked from commit 1e7a1d9)
@clackary clackary mentioned this pull request Feb 21, 2025
Closed
@asl asl mentioned this pull request Apr 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavapestov slavapestov slavapestov approved these changes

@hamishknight hamishknight hamishknight approved these changes

@rxwei rxwei Awaiting requested review from rxwei

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@jckarter jckarter Awaiting requested review from jckarter jckarter is a code owner

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash in silgen due to missed / failed typecheck
3 participants
@asl @slavapestov @hamishknight