The last line of a returning function isn't executed if it isn't stored somewhere

[marcusnaslund]

Consider a very simple stack:

SimpleStack: class <T> {
    _data: T*
    _count: SizeT = 0
    _capacity: SizeT = 8
    init: func { this _data = calloc(this _capacity, T size) }
    push: func (element: T) {
        this _data[this _count] = element
        this _count += 1
    }
    pop_broken: func -> T {
        this _data[--this _count]
    }
    pop_works: func -> T {
        this _count -= 1
        this _data[this _count]
    }
}

Popping the stack using temp := myStack pop_broken() works, but if the line is simply myStack pop_broken(), then his _data[--this _count] isn't executed at all (and thus this _count is never decreased). The pop_works method always works because this _count is decreased, and then the last (returning) line is not executed, but that's fine, since we don't care about it.

A simple example demonstrating what works and what does not:

// This does not work
stack_broken := SimpleStack<Int> new()
stack_broken push(1) . push(2) . push(3)
a := stack_broken pop_broken()
stack_broken pop_broken() // The last line of pop is never run because the return value isn't stored
b := stack_broken pop_broken()

a toString() println() // Should be 3, is 3
b toString() println() // Should be 1, is 2 :(

// Here is a working way
stack_works := SimpleStack<Int> new()
stack_works push(1) . push(2) . push(3)
a = stack_works pop_works()
stack_works pop_works() // Here, the return value isn't stored, but it works anyway because _count is decreased before the last line of the method
b = stack_works pop_works()

a toString() println() // Should be 3, is 3
b toString() println() // Should be 1, is 1 :)

[alexnask]

Will take a look at it today.
Sounds weird, probably some generics bug (what I'm thinking is happening is that the whole function becomes a single return statement and isn't executed when you don't store the result).

[thomasfanell]

@shamanas I ran into something similar a few weeks ago, and I found that the generated C-code is checking if the receiver is a null pointer, and if it is, nothing is done. That is, instead of a pointer to an lvalue, Literal NULL is passed to the generated C-function.

[thomasfanell]

void stack_pop(stack_t* stack, int* returnValue) {
    if (returnValue != NULL) {
        // do stuff
    }
}

and the generated call without an lvalue would be: stack_pop(theStack, NULL)

[alexnask]

Yes, generic return is implementing by taking a generic pointer for output, so if we discard the return value we pass NULL, than NULL checks are done around every return statement, so it's exactly like I suspected.

Now, the solution is obvious:
Instead of just generating an if statement, also generate an else branch that just executes the expression.

[alexnask]

Actually, it looks like rock already tries to take this into account but hasSideEffects doesn't detect the side effects here :P

[alexnask]

@marcusnaslund By the way, there is no decrement or increment operators in ooc (postfix or prefix), prefix operators just incidentally work because we have prefix + and - and it is apparently legal to have more than one.
Should probably fix that too.

[vendethiel]

There's no side effects in pop_broken - if you don't store the result, it's a no-op (it isn't currently due to OOC emitting "--" in C, but semantically should be)

[alexnask]

Yeah, I agree, I am talking about the exact same code replaced with the correct way to do things this count -= 1.

hasSideEffects always returned false for array accesses.

[alexnask]

Also, BinaryOp returned the opposite of what it should (it returned !isAssign(), while it should return isAssign())

[vendethiel]

Ah, makes sense