Dividing variable by itself in loop_handler.go

[McLaynV]

Is there an existing issue for this?

• I have searched the existing issues

Version of Corteza

2024.9.x branch

Current Behavior

In server/automation/automation/loop_handler.go there is:

if args.First*(args.Step/args.Step) >= args.Last*(args.Step/args.Step)

corteza/server/automation/automation/loop_handler.go

Line 42 in 31d38a2

if args.First*(args.Step/args.Step) >= args.Last*(args.Step/args.Step) {

Expected Behavior

I believe it is not intended to compute args.Step/args.Step.
If it is intended, there should be a comment explaining what magic is going on.

Steps To Reproduce

No response

Environment and versions

No response

Anything else?

@darh

[github-actions]

Stale issue message

[McLaynV]

@darh or @tjerman could you have a look at it?

loopSequenceArgs.Step is of type int64. That gives us 2 possible scenarios:

• if Step == 0, then Step/Step causes runtime panic. Returning an error would be better.
• if Step != 0, then Step/Step is 1 and the x*(args.Step/args.Step) reduces to x*1 which is plain x.

---

So to keep the current functionality, just replace

	if args.First*(args.Step/args.Step) >= args.Last*(args.Step/args.Step) {
		return nil, fmt.Errorf("failed to initialize counter iterator with first step greater than last")
	}

with

	if args.Step == 0 {
		return nil, fmt.Errorf("failed to initialize counter iterator with zero step")
	}
	if args.First >= args.Last {
		return nil, fmt.Errorf("failed to initialize counter iterator with first step greater than last")
	}

---

I can imagine it was meant to be a sign check to allow negative steps. If so, it could be like this:

func signOf(n int64) int {
    if n > 0 {
        return 1
    } else if n < 0 {
        return -1
    }
    return 0
}

and replace

	if args.First*(args.Step/args.Step) >= args.Last*(args.Step/args.Step) {
		return nil, fmt.Errorf("failed to initialize counter iterator with first step greater than last")
	}

with

	if args.Step == 0 && args.First != args.Last {
		return nil, fmt.Errorf("failed to initialize counter iterator with zero step")
	}
	sign := signOf(args.Step)
	if args.First*sign > args.Last*sign {
		return nil, fmt.Errorf("failed to initialize counter iterator with first value greater than last")
	}

or if zero steps are not allowed, then replace the block with

	sign := signOf(args.Step)
	if args.First*sign >= args.Last*sign {
		return nil, fmt.Errorf("failed to initialize counter iterator with first value greater than last")
	}

[tjerman]

It should be

        if step == 0 {error...}

        sign := signOf(args.Step)
	if args.First*sign >= args.Last*sign {
		return nil, fmt.Errorf("failed to initialize counter iterator with first value greater than last")
	}

If you want, do a pull request so I don't steal your work

[McLaynV]

No problem @tjerman I grant you permission to steal this piece of code from me :D

I don't have a setup to build go or to run the tests. So I would only feel bad that I can't check all the checkboxes in the PR template.