Stopgap make_zero bugfix #1961
Stopgap make_zero bugfix #1961
Conversation
Aiming for full coverage of both new and old implementations of make_zero(!)
| EnzymeCore.make_zero(Core.Typeof(prev2), seen, prev2, Val(copy_if_inactive)), | ||
| ) | ||
| seen[prev] = res |
There was a problem hiding this comment.
this ordering feels weird, what if a box is in a recursive datastructure?
There was a problem hiding this comment.
Hm, I don't see what's wrong? The first time you encounter this prev, wherever it lives in the datastructure, you construct the corresponding zeroed value, and then you store it in the IdDict such that if the same identical prev is encountered again, make_zero will return the same identical res
There was a problem hiding this comment.
Oh, I see what you mean now, self-referential data structures hadn't even crossed my mind while working on this. Will make sure to add tests for that to whichever PR gets merged.
There was a problem hiding this comment.
The actual point of the change here was to get rid of the spurious Ref. The reordering was collateral.
| return prev | ||
| # Unclear what types might end up here rather than in specialized methods or | ||
| # guaranteed_const_nongen, but as a last-ditch attempt try falling back to Base.zero | ||
| return Base.zero(prev)::RT |
There was a problem hiding this comment.
if it has no fields to zero, we should use the old object here
|
Thanks for taking a look! I noticed a few other issues that should be fixed too. However, #1852 is almost there (I've been in a cycle of thorough tests => noticing issues => fixing/improving design accordingly => repeat), so if you don't mind I'll finish the big push on that, and then we can decide whether it's worth fixing and merging this one first (say, if reviewing #1852 is too big of a task to deal with right now), or if we should close this one and focus on #1852 |
Since #1852 isn't quite ready, in the meantime, here's a PR fixing the bugs I found in make_zero(!) and adding a fairly thorough test suite.
The last test currently fails due to #1935. I didn't want to mark it as broken because I'd rather see #1936 merged (or #1935 fixed in some other way), but let me know if I should change that.