Accessing array[-1] element

In svd.go:194:

Line 194 in 845d02e

if math.Abs(S[L-1]) <= eps {

The last loop iteration is L=0, and in this case S[-1] is attempted to be accessed, which is undefined behaviour.

Additionally, after the L=0 loop, L will be decremented to L=-1, which causes the loop condition to fail, and thus exit the loop. The following loop on line 203 sets i=L=-1, and thus attempts to access t[-1], which is undefined behaviour.

Suggested fix for line 190 is to make it closer to the original Fortran:

L = k
for LL = 1; LL <= k; LL++ {
    L = k + 1 - LL

Interestingly, an older C translation of SVD from the ACM 1969 issue has the same bug, but not the original Fortran...
http://www.geocities.ws/hjsmithh/Algo358GDa.zip

Thank you for pointing this out.
Are you sure negative indexes are undefined? I thought that will panic.
Did it fail for you? I never had an issue.

The similarity with the C reference is maybe not a coincidence. I actually used a C port as the basis, mentioned somewhere in a forum. But I could not find it again. It's possible that it's this one.

Doesn't the fortran code has the same issue?

do ll = 1, k
          l = k + 1 - ll
          if ( abs ( t(l) ) .le. eps ) then
                  go to 290
          end if
          if ( abs ( s(l-1) ) .le. eps ) then
                  go to 240
          end if
end do

For the last step in the loop ll=k and l=1.
The indexing s(l-1) is 0, but fortran indexes start at 1, right?

I think this is not an issue, because for L=0 the first condition is always satisfied:

if math.Abs(t[L]) <= eps // goto Test

t is set to c at svd.go#L172, and c[0] is never assigned so it is always 0.
Do you agree?

Haha, I don't know Fortran except for 5 mins of research I did when attempting to interpret it just before. It seems you're right that arrays start indexing at 1 by default, unless otherwise specified (https://web.stanford.edu/class/me200c/tutorial_90/07_arrays.html).

I found it from a static analyser highlighting the issue in the C code I referenced. I wasn't able to come up with input data that I could see trigger the bad behaviour. In the C code it did have c[0] = 0.0, and later t[k] = c[k] for k=0. Yet the static analyser wasn't smart enough to apply the abs(t[0]) and determine it breaks the loop like you say. In truth, I have not even used Go, so if you say variables are all set to 0 initially, then I'll take your word for it.

If you're comfortable with relying on if math.Abs(t[L]) <= eps // goto Test always breaking the loop for L = 0, then that's fine.. but it is less clear than the suggested code I had above.

I agree that it is not a nice or obvious formulation.
But I'll keep the current code, as it is similar to the original. This should be more a transpilation than a re-implementation.

Go variables are always initialized with the zero value:
https://golang.org/ref/spec#The_zero_value
This is part of the language and people use this a lot.

Thank's again for showing the issue.