Replace type variables in a single sweep during reification #1888
Conversation
plietar
added
the
changelog - fixed
packages/term/ansi_term.pony
Outdated
| @@ -80,7 +80,8 @@ actor ANSITerm | |||
| return | |||
| end | |||
|
|
|||
| for c in (consume data).values() do | |||
| let data' : Array[U8] ref = consume data | |||
There was a problem hiding this comment.
Sorry, a small style consistency point - could you remove the space preceding the colon here, and in the other three places in this commit where it happens?
|
The C changes look good to me. However, I don't think we should merge this until we have a plan for #1887, as this will break some people's code and we don't have a plan for the permanent fix yet. |
|
Yeah, that sounds reasonable. |
|
This has come up again in #1921. The way I see it, with this bug the compiler is currently doing a type substitution which is just wrong, and it's only a coincidence that this substitution allows the standard library to build today. |
|
@plietar this is tagged as "CHANGELOG-FIXED". When this is merged the CHANGELOG comment would be "Replace type variables in a single sweep during reification" which I don't think is a not of value in a CHANGELOG. Ideally this would have a changelog comment that is meaningful to the average Pony user. I'd be in favor of either a) changing the title of this PR |
|
Did we ever get @sylvanc's thoughts on #1887? I'm still not very comfortable merging this until we have a plan of action for #1887. I think @sylvanc will have the best big-picture view of the original intent behind some of this stuff, as well as some other important insights to coming up with the right solution. |
|
We discussed #1887 on a call - we can indeed replace |
SeanTAllen
added
do not merge
|
...and we should have the #1887 fix first, so that the manual rcap decay used here isn't needed. |
mfelsche
added
bug: 4 - in progress
and removed
do not merge
|
Again, like in #2503 i took the freedom to rebase this on master in order to get this rolling again. It seems i broke something within the stdlib though, fixing. |
|
@mfelsche any progress on this? it's been a while. |
|
Unfortunately not. It seems i broke it and i dont know why yet. Bein realistic here, i dont think i will manage it in the next few weeks. :( |
|
Currently blocked |
|
New PR: |
Fixes #1875
The first commit is a workaround for #1887. It can be removed when that issue is fixed (depending on how it is fixed). I don't know if we should wait for it to be fixed first.