Hi Waldek, it looks like
https://github.com/fricas/fricas/commit/25ebb18781cfe2c21f053a8a30086b66cd3724f3#diff-bcfc7730b623452ab2a4689c03f7923aL294 is the first part of your patch where you actually only refactored the code and the second part starts further below at line 317 https://github.com/fricas/fricas/commit/25ebb18781cfe2c21f053a8a30086b66cd3724f3#diff-bcfc7730b623452ab2a4689c03f7923aL317 Can you confirm? I just try to understand the first part of the change in the lift? function. The original code says... 1- leadpol : Boolean := false 2- (listpol, lval) := (uterm.lpol, uterm.lint.first) 3- d := listpol.first 4- listpol := listpol.rest 5- nolift : Boolean := true 6- for uf in listpol repeat 7- --note uf and d not necessarily primitive 8- degree gcd(uf, d) =0 => nolift := false 9- nolift => ["notCoprime"] 10- f : SUPP := ([p1, p2]$List(SUPP)).(position(uf, listpol)) 11- lgcd := gcd(leadingCoefficient p1, leadingCoefficient p2) 12- (l := lift(f,d,uf,lgcd,lvar,ldeg,lval)) case "failed"=>["failed"] 13- [l :: SUPP] The new code is: 1+ (listpol, lval) := (uterm.lpol, first(uterm.lint)) 2+ d := first(listpol) 3+ listpol := rest(listpol) 4+ uf := listpol(1) 5+ f := p1 6+ --note uf and d not necessarily primitive 7+ if degree gcd(uf, d) ~= 0 then 8+ uf := listpol(2) 9+ f := p2 10+ if degree gcd(uf, d) ~= 0 then return ["notCoprime"] 11+ lgcd := gcd(leadingCoefficient p1, leadingCoefficient p2) 12+ l := lift(f, d, uf, lgcd, lvar, ldeg, lval) 13+ l case "failed" => ["failed"] 14+ [l :: SUPP] Removing 1- is clear. The following lines are equivalent. 2- and 1+, 3- and 2+, 4- and 3+, 12- and (12+,13+), 13- and 14+. Then obviously both versions assume that uterm.lpol is a list that contains at least one element. I haven't yet checked the context whether this is safe to assume. Do you know? Actually the old code also assumes that the list has at least length 2. Because otherwise uf has no value in 10-. In fact, the old code is bad anyway, since I consider that the loop variable is no longer available after the loop. Also I would miss a 'break' in the loop, because the first case where degree gcd(uf, d) =0 can abort the loop. With '=>' just the rest of the body is skipped and the next iteration starts. For that reason in the old code, uf was always listpol.(#listpol). But there is one case (in the old code) where uf is not needed. Namely, if #(uterm.lpol)=1, the for loop body is never executed so nolift is still true and lift? exists with ["notCoprime"]. In the new code, line 4+ tells me that #(uterm.lpol)>=2 unconditionally. Again, context might say that this is always the case, but since there is no input/output specification for lift?, I cannot easily verify this as a reviewer and must invest time to see whether this is indeed safe to assume and actually the intention of the lift? function. :-( Furthermore, if degree gcd(uf, d) ~= 0, the new code assumes in line 8+ even #(uterm.lpol)>=3. Since 3 feels a bit strange in this place. I have already doubts that the new code is correct and does not lead to a failure while accessing listpol(2), but my analysis is not over. Without investigating what the input conditions are, I cannot really claim that I've found a bug. At least the old code and the new code are not equivalent, so the first part of your patch is not just refactoring it, but also a change in the semantics of this function. Waldek, can you say why this semantic change makes sense? Another case where the new code behaves different from the old code is the case #(listpol:=uterm.lpol)>=4 degree gcd(listpol.2, listpol.1) ~= 0 degree gcd(listpol.3, listpol.1) ~= 0 degree gcd(listpol.4, listpol.1) = 0 In that case the new code returns ["notCoprime"] whereas the old code detects that there is a zero degree and comes until 13-. Waldek, different from my initial thought, that means that this first part is not the refactoring part, but also is part of the bugfix? So what is actually refactoring and what is bugfix? Ralf -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/fricas-devel. For more options, visit https://groups.google.com/d/optout.
