[Bounty] Fixes attempts at taking rvalue references when dealing with cover properties

[alexnask]

Description:
Tightened up the way rock decides whether an expression is referencable.
In particular, non extern const accesses where always deemed referencable (extern const values are presumed to be defined constants).

However, this is untrue if the expression of the access is non-referencable itself.
In addition to that, (at least in standard ooc), ooc array accesses cannot have their address taken but the compiler claimed all array accesses could do so.

[thomasfanell]

C arrays appears to be broken:

populate: func (array: Int*, length: Int) {
    for (i in 0 .. length)
        array[i] = i
}

array: Int[3]
populate(array[0]&, 3)

for (i in 0 .. 3) "i = %d" printfln(array[i])
/*
    i = 0
    i = 0
    i = 0
*/

Upstream rock prints the expected output:

/*
    i = 0
    i = 1
    i = 2
*/

[alexnask]

@thomasfanell Thanks, will be looking into it later today.

[thomasfanell]

@shamanas any luck with this one? (no rush)

[alexnask]

@thomasfanell
Took a look the other, couldn't figure out why this code doesn't work.
Will be active this weekend and figure this out, as well as some other over due bugs.

[alexnask]

Hmm, weird, the output I get is:

i = 1
i = 2
i = 0

Which is of course also wrong. Officially investigating! :p

[alexnask]

Rock incorrectly marks 'array[0]' as an unreferencable expression and thus creates an intermediate variable and takes it's address (which of course is incorrect here).

This is because the change wrongly assumes all ArrayType expressions are ooc arrays.

[thomasfanell]

@shamanas Thanks, I will have a look over the weekend

[thomasfanell]

@shamanas have you tried compiling and testing ooc-kean with these changes? Something is causing threading not to work, it seems to deadlock no matter what.

If you want to try it for yourself (in ooc-kean root): ./test.sh concurrent/Thread To test the entire namespace: ./test.sh concurrent

[alexnask]

@thomasfanell
Hm, interesting.
I will be taking a look tomorrow afternoon.

EDIT: Looking into it now.

[alexnask]

@thomasfanell
I'm having issues because of the extend change, since ooc-kean still uses the extention of specific types workaround, haven't found a branch that updated this, I will probably fork it into one of my own branches and try to find the issues you are talking about.

[thomasfanell]

@shamanas I think both of these could work: https://github.com/thomasfanell/ooc-kean/tree/generic-extensions-fix https://github.com/thomasfanell/ooc-kean/tree/adapt_for_rock_1.0.21

[alexnask]

@thomasfanell
Ah, didn't look for forks, I did it on my own branch anyways :)
The tests seem to be running fine on my setup though (apart from BlockedQueue).
I will be taking the diff of the C sources generated with and without the fix to find out what is happening.

[alexnask]

So it turns out this code has a non critical bug (some variable accesses generate intermediate variables to then take their address, I don't know why I thought that was a good idea at the time).

However, I don't see anything that could cause the ooc-kean tests to fail (and as I said, they work fine on my system).
Here is the whole diff of the rock_tmp folders generated with ./test.sh concurrentwith and without this fix applied:

diff -r magic_rock_tmp/ooc/concurrent/native/ConditionUnix.c rvalue_rock_tmp/ooc/concurrent/native/ConditionUnix.c
75c75,81
<     return external_pthread__pthread_cond_wait(this->_backend, &(((native_MutexUnix__MutexUnix*) (mutex))->_backend)) == 0;
---
>     
>     {
>         #line 30 "C:\\dev\\ooc_libs\\ooc-kean\\source\\concurrent\\native\\ConditionUnix.ooc"
>         external_pthread__PThreadMutex __native_ConditionUnix_unreferencable3 = ((native_MutexUnix__MutexUnix*) (mutex))->_backend;
>         #line 30 "C:\\dev\\ooc_libs\\ooc-kean\\source\\concurrent\\native\\ConditionUnix.ooc"
>         return external_pthread__pthread_cond_wait(this->_backend, &(__native_ConditionUnix_unreferencable3)) == 0;
>     }
diff -r magic_rock_tmp/ooc/system/structs/HashMap.c rvalue_rock_tmp/ooc/system/structs/HashMap.c
456c456
<     __structs_HashMap_structs_HashMap_closure21_ctx* __structs_HashMap_ctx22 = Memory__calloc(1, ((types__Class*)__structs_HashMap_structs_HashMap_closure21_ctx_class())->size);
---
>     __structs_HashMap_structs_HashMap_closure22_ctx* __structs_HashMap_ctx23 = Memory__calloc(1, ((types__Class*)__structs_HashMap_structs_HashMap_closure22_ctx_class())->size);
458c458
<     (*(__structs_HashMap_ctx22)) = (__structs_HashMap_structs_HashMap_closure21_ctx) { 
---
>     (*(__structs_HashMap_ctx23)) = (__structs_HashMap_structs_HashMap_closure22_ctx) { 
462,464c462,464
<     types__Closure __structs_HashMap_closure23 = (types__Closure) { 
<         structs_HashMap____structs_HashMap_structs_HashMap_closure21_thunk, 
<         __structs_HashMap_ctx22
---
>     types__Closure __structs_HashMap_closure24 = (types__Closure) { 
>         structs_HashMap____structs_HashMap_structs_HashMap_closure22_thunk, 
>         __structs_HashMap_ctx23
467c467
<     types__Closure writerFunc = __structs_HashMap_closure23;
---
>     types__Closure writerFunc = __structs_HashMap_closure24;
584,585d583
<             String__String* __structs_HashMap_genCall17 = NULL;
<             #line 242 "C:\\dev\\ooc_libs\\ooc-kean\\source\\system\\structs\\HashMap.ooc"
586a585,586
>             #line 242 "C:\\dev\\ooc_libs\\ooc-kean\\source\\system\\structs\\HashMap.ooc"
>             String__String* __structs_HashMap_genCall19 = NULL;
588c588
<             String__String* __structs_HashMap_genArg19 = String__String_clone((structs_VectorList__VectorList___OP_IDX_Int__T(pair, (uint8_t*) &(__structs_HashMap_genCall17), 0), __structs_HashMap_genCall17));
---
>             String__String* __structs_HashMap_genArg20 = String__String_clone((structs_VectorList__VectorList___OP_IDX_Int__T(pair, (uint8_t*) &(__structs_HashMap_genCall18), 0), __structs_HashMap_genCall18));
590c590
<             String__String* __structs_HashMap_genArg20 = String__String_clone((structs_VectorList__VectorList___OP_IDX_Int__T(pair, (uint8_t*) &(__structs_HashMap_genCall18), 1), __structs_HashMap_genCall18));
---
>             String__String* __structs_HashMap_genArg21 = String__String_clone((structs_VectorList__VectorList___OP_IDX_Int__T(pair, (uint8_t*) &(__structs_HashMap_genCall19), 1), __structs_HashMap_genCall19));
592c592
<             structs_HashMap__HashMap_put(result, NULL, (uint8_t*) &(__structs_HashMap_genArg19), (uint8_t*) &(__structs_HashMap_genArg20));
---
>             structs_HashMap__HashMap_put(result, NULL, (uint8_t*) &(__structs_HashMap_genArg20), (uint8_t*) &(__structs_HashMap_genArg21));
611c611
<     Numbers__Int __structs_HashMap_match16;
---
>     Numbers__Int __structs_HashMap_match17;
615c615
<         __structs_HashMap_match16 = (* (Numbers__Int*)key);
---
>         __structs_HashMap_match17 = (* (Numbers__Int*)key);
620c620
<         __structs_HashMap_match16 = structs_HashMap___acX31Hash((types__Class*)String__String_class(), (uint8_t*) &(__structs_HashMap_genArg15));
---
>         __structs_HashMap_match17 = structs_HashMap___acX31Hash((types__Class*)String__String_class(), (uint8_t*) &(__structs_HashMap_genArg15));
621a622,623
>         #line 1 "C:\\dev\\ooc_libs\\ooc-kean\\source\\system\\structs\\HashMap.ooc"
>         OwnedBuffer__OwnedBuffer __structs_HashMap_genArg16 = (* (Text__Text*)key)._buffer._backend;
623c625
<         __structs_HashMap_match16 = structs_HashMap___acX31Hash((types__Class*)OwnedBuffer__OwnedBuffer_class(), (uint8_t*) &((* (Text__Text*)key)._buffer._backend));
---
>         __structs_HashMap_match17 = structs_HashMap___acX31Hash((types__Class*)OwnedBuffer__OwnedBuffer_class(), (uint8_t*) &(__structs_HashMap_genArg16));
626c628
<         __structs_HashMap_match16 = structs_HashMap___murmurHash((types__Class*)K, (uint8_t*) key);
---
>         __structs_HashMap_match17 = structs_HashMap___murmurHash((types__Class*)K, (uint8_t*) key);
629c631
<     Numbers__Int result = __structs_HashMap_match16;
---
>     Numbers__Int result = __structs_HashMap_match17;
665c667
< void __structs_HashMap_structs_HashMap_closure21_ctx___cover_defaults__(__structs_HashMap_structs_HashMap_closure21_ctx* this) {
---
> void __structs_HashMap_structs_HashMap_closure22_ctx___cover_defaults__(__structs_HashMap_structs_HashMap_closure22_ctx* this) {
667c669
< void __structs_HashMap_structs_HashMap_closure21_ctx___load__() {
---
> void __structs_HashMap_structs_HashMap_closure22_ctx___load__() {
671c673
< __structs_HashMap_structs_HashMap_closure21_ctxClass *__structs_HashMap_structs_HashMap_closure21_ctx_class(){
---
> __structs_HashMap_structs_HashMap_closure22_ctxClass *__structs_HashMap_structs_HashMap_closure22_ctx_class(){
673c675
<     static __structs_HashMap_structs_HashMap_closure21_ctxClass class = 
---
>     static __structs_HashMap_structs_HashMap_closure22_ctxClass class = 
678,679c680,681
<                     .instanceSize = sizeof(__structs_HashMap_structs_HashMap_closure21_ctx),
<                     .size = sizeof(__structs_HashMap_structs_HashMap_closure21_ctx)
---
>                     .instanceSize = sizeof(__structs_HashMap_structs_HashMap_closure22_ctx),
>                     .size = sizeof(__structs_HashMap_structs_HashMap_closure22_ctx)
691c693
<         classPtr->name = (void*) String__makeStringLiteral("__structs_HashMap_structs_HashMap_closure21_ctx", 47);
---
>         classPtr->name = (void*) String__makeStringLiteral("__structs_HashMap_structs_HashMap_closure22_ctx", 47);
740c742
<         __structs_HashMap_structs_HashMap_closure21_ctx___load__();
---
>         __structs_HashMap_structs_HashMap_closure22_ctx___load__();
854c856
< void structs_HashMap____structs_HashMap_structs_HashMap_closure21(io_FileWriter__FileWriter* fileWriter, String__String** keyPtr, String__String** valuePtr) {
---
> void structs_HashMap____structs_HashMap_structs_HashMap_closure22(io_FileWriter__FileWriter* fileWriter, String__String** keyPtr, String__String** valuePtr) {
869c871
< void structs_HashMap____structs_HashMap_structs_HashMap_closure21_thunk(String__String** keyPtr, String__String** valuePtr, __structs_HashMap_structs_HashMap_closure21_ctx* __context__) {
---
> void structs_HashMap____structs_HashMap_structs_HashMap_closure22_thunk(String__String** keyPtr, String__String** valuePtr, __structs_HashMap_structs_HashMap_closure22_ctx* __context__) {
871c873
<     structs_HashMap____structs_HashMap_structs_HashMap_closure21((*__context__).fileWriter, keyPtr, valuePtr);
---
>     structs_HashMap____structs_HashMap_structs_HashMap_closure22((*__context__).fileWriter, keyPtr, valuePtr);

As you can see, most are closure names changing because of two extra intermediate variables being created with the fix.

[alexnask]

With this commit the diff is null (/empty/however they call those).

[thomasfanell]

@shamanas all tests pass here now, good job! :+1: I have some more testing to do, and if all goes well, I'll ask @fredrikbryntesson to sign off on this

[marcusnaslund]

:tada:

[fredrikbryntesson]

Cool

[thomasfanell]

@fredrikbryntesson

[fredrikbryntesson]

@shamanas Sent you a mail :)