Replace without stack overflow # 25384 #25386
Conversation
|
Is the first commit here intended to be in this PR? |
|
@rfourquet no - the first commit should be unrelated and forms a separate PR. |
base/set.jl
Outdated
|
|
||
|
|
||
| replace!(prednew::Callable, A; count::Int=typemax(Int)) = | ||
| throw(MethodError(replace!, (prednew, A, count))) |
There was a problem hiding this comment.
Missing indentation. Also, count is not a positional argument, so I don't think it should be passed to MethodError (I'm not sure how it could be passed).
There was a problem hiding this comment.
The first commit was by error - I will back that out.
Next commit will include your suggestions.
|
Makes sense to me, but why is the first included here? |
|
Oh dear, this causes a method overwrite that I didn't notice at first. This will have to be reverted. The many signatures of this function really make my head hurt. There are about 15 barely-distinguishable method signatures here. Surely we can simplify this somehow? |
`replace!` had one stack-overflow problem reported in #25384. Its fix #25386 made a method-overwrite, tentatively fixed in #25422. But `replace!` also didn't handle non-Int count arguments as expected (because there is no dispatch on keyword arguments). This fixes the root problem and adds tests for non-Int count.
Then you are probably talking about |
`replace!` had one stack-overflow problem reported in #25384. Its fix #25386 made a method-overwrite, tentatively fixed in #25422. But `replace!` also didn't handle non-Int count arguments as expected (because there is no dispatch on keyword arguments). This fixes the root problem and adds tests for non-Int count.
`replace!` had one stack-overflow problem reported in #25384. Its fix #25386 made a method-overwrite, tentatively fixed in #25422. But `replace!` also didn't handle non-Int count arguments as expected (because there is no dispatch on keyword arguments). This fixes the root problem and adds tests for non-Int count.
`replace!` had one stack-overflow problem reported in #25384. Its fix #25386 made a method-overwrite, tentatively fixed in #25422. But `replace!` also didn't handle non-Int count arguments as expected (because there is no dispatch on keyword arguments). This fixes the root problem and adds tests for non-Int count.
4 participants
#25384
a dummy version of
replace!has been added, which throwsMethodErrorfor unsupported argument types.