-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[TypeChecker] SE-0213: Implement literal init via coercion #17860
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
Changes from all commits
1e4e965
aa9b3d8
9371aaa
16e2cd1
29bbc99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3591,10 +3591,12 @@ ${assignmentOperatorComment(x.operator, True)} | |
/// to find an absolute value. In addition, because `abs(_:)` always returns | ||
/// a value of the same type, even in a generic context, using the function | ||
/// instead of the `magnitude` property is encouraged. | ||
@_transparent | ||
public var magnitude: U${Self} { | ||
let base = U${Self}(_value) | ||
return self < (0 as ${Self}) ? ~base + 1 : base | ||
@inline(__always) | ||
get { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems unrelated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed because constant propagation now looks at both branches, so as a way to work around that behavior @ravikandhadai and @moiseev told me that it would be appropriate to switch from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, unfortunately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify. The changes to how initializers are resolved, as a side-effect, makes constant propagation more precise as it can now propagate through more initializers. While in general it is a good thing, in this specific case, it resulted in a false positive due to mandatory inlining.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not write There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is very much possible (and probably reasonable) to try the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @moiseev @xwu there is one other issue with "magnitude" (besides the +) that could trigger static errors if it is made @_transparent. It is passing a potentially negative (builtin Int) into a UInt : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xwu Ah okay, this initializer should not be a problem for constant prop. There will no static errors here. Okay we can try making + to &+ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to open follow up PR for this and run the benchmarks |
||
let base = U${Self}(_value) | ||
return self < (0 as ${Self}) ? ~base + 1 : base | ||
} | ||
} | ||
% end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,8 +109,5 @@ func testIntToFloatConversion() { | |
let e2: Float80 = 18_446_744_073_709_551_617 // expected-warning {{'18446744073709551617' is not exactly representable as 'Float80'; it becomes '18446744073709551616'}} | ||
_blackHole(e2) | ||
|
||
// No warnings are emitted for conversion through explicit constructor calls. | ||
// Note that the error here is because of an implicit conversion of the input | ||
// literal to 'Int'. | ||
_blackHole(Float80(18_446_744_073_709_551_617)) // expected-error {{integer literal '18446744073709551617' overflows when stored into 'Int'}} | ||
_blackHole(Float80(18_446_744_073_709_551_617)) // Ok | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xedin Nice to see that the above FIXME's are addressed by this fix. Thanks! |
||
} |
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.
This is the use of string literals for selectors? I wonder if we can (separately) kill that now...
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.
That's right, it's
#selector(...)
vs.Selector("...")