-
Notifications
You must be signed in to change notification settings - Fork 10.5k
AST: Don't set source location on cloned parameter lists #31778
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
AST: Don't set source location on cloned parameter lists #31778
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1,7 @@ | |||
// RUN: %target-swift-frontend -emit-silgen %S/Inputs/partial_apply_debuginfo_other.swift -primary-file %s -module-name partial_apply_debuginfo -g -Xllvm -sil-print-debuginfo | %FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should ideally be positive. Is there a visible difference in a loc
attachment on either a debug_value or an alloc_stack that we could check for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see one, in fact this test passes without my change, which is why I asked :) I'm not going to merge this in its current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you perhaps see a difference with -verbose-sil? This also prints the non-debug-info relevant details of SILLocation.
bd96d04
to
ffd92f9
Compare
@swift-ci Please smoke test |
@swift-ci Please smoke test macOS |
@swift-ci Please smoke test Linux |
@swift-ci Please clean smoke test Linux |
This seems to have introduced a regression on the windows builds: https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/1538/changes#detail
|
Note that rdar://problem/63191951 was actually fixed by #31992, and not this change as I expected. However, this change makes sense anyway, and this PR adds the missing test case.