Swift: extract types for patterns#14570
Conversation
AlexDenisov
left a comment
There was a problem hiding this comment.
I'm not sure about the upgrade script, but overall LGTM.
Happy to approve once the formatting is fixed.
| <dbstats> | ||
| <typesizes /> | ||
| <stats /> | ||
| </dbstats> |
There was a problem hiding this comment.
We didn't put stats in any other upgrade scripts, do we need this now?
There was a problem hiding this comment.
I don't think we do, no.
geoffw0
left a comment
There was a problem hiding this comment.
Results look good as far as I can see.
It would be good to understand the motivation for this and whether this change meets that need. I can certainly imagine these types being useful.
I also think we should have a DCA run on this.
The motivation for this is for type pruning, where we need to assign a meaningful type for dataflow nodes. Some time ago I talked to Robert about what we should do for the dataflow nodes representing patterns, and we agreed that we could probably assign it some notion of a Top type, but it looks like we might as well grab the real type off the patterns (which is what Robert is extracting here).
Very much agree! |
|
DCA looks clean, only thing left is that question about the upgrade script - @AlexDenisov is there a better way to synthesize an empty table? |
I don't know, and I also don't know why we'd need to synthesize a new table 😅 |
|
I think the |
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
dc278b8 to
0dc4a68
Compare
geoffw0
left a comment
There was a problem hiding this comment.
There's still a spurious swift.dbscheme.stats in the upgrade script.
Assuming the upgrade / downgrade scripts still connect to the right versions, this otherwise LGTM.
|
I'm ready to approve this when the spurious |
|
Lots of noise in the DCA analysis timings, but mostly faster and all with high standard deviations. I don't see any red flags. |
No description provided.