Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generic-arrays #41

Closed
wants to merge 2 commits into from
Closed

Conversation

horasal
Copy link

@horasal horasal commented Mar 11, 2016

Description

This pull request allow the following code work:

array := GenericClass<Int>[10] new()

In detail, the above code will be unwrapped to:

array := Array new(GenericClass*, 10)

Implementation Details

In current rock, only ArrayAccess is automatically unwrapped to creation. However, GenericClass<TypeArg>[Expression] will be parsed as a TypeAccess of ArrayType. It is clearly that calling methods on ArrayType can not get anything. Thus, before FunctionCall starts resolve, TypeAccess should be turned into an array creation.

Here the unwrap process happens before resolving expr. This is because that we only need to know the type of expr and inner, which can be judged by instanceOf?.

@horasal
Copy link
Author

horasal commented Mar 11, 2016

Test, please notice that ExampleClass<Int>[10] new() is just for creating the array, where its elements remain un-initialized:

ExampleClass: class <T> {
    member: T
    totalCount: static Int = 0
    count: Int
    init: func {
        This totalCount += 1
        this count = This totalCount
    }
    increase: func -> Int {
        this count += 1
        this count
    }
}

array := ExampleClass<Int>[10] new()
for (i in 0 .. 10) {
    array[i] = array[i] new()
    array[i] increase()
    raise(array[i] count != i + 2, "Element at %d had count=%d" format(i, array[i] count))
}
array free()

@thomasfanell
Copy link

I would expect this to work:

GenericClass: class <T> {
    value: T
    init: func (=value)
}

arrayOfGenericClass := GenericClass<Int>[5] new()
// Initialize array elements
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] = GenericClass<Int> new(i + 1)
for (i in 0 .. arrayOfGenericClass length)
    "%d" printfln(arrayOfGenericClass[i] value)
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] free()
arrayOfGenericClass free()

/*
Output:
  5
  -241166464
  -241166464
  -241166464
  -241166464
*/

If I instead print the value immediately after initializing the array item (in the first loop), it works as expected.

@horasal
Copy link
Author

horasal commented Mar 12, 2016

Compiling the following code with magic-lang/rock:master gives the same error. I think it is not a bug introduced by this pull request.

Anyway, I will take a look at it.

GenericClass: class <T> {
    value: T
    init: func (=value)
}

arrayOfGenericClass := [GenericClass<Int> new(1),GenericClass<Int> new(2),GenericClass<Int> new(3),GenericClass<Int> new(4),GenericClass<Int> new(5)]
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] = GenericClass<Int> new(i + 1)
for (i in 0 .. arrayOfGenericClass length){
    arrayOfGenericClass[i] value toString() println()
}
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] free()
arrayOfGenericClass free()

/*
Output:
  5
  -241166464
  -241166464
  -241166464
  -241166464
*/

@horasal
Copy link
Author

horasal commented Mar 12, 2016

Can also be reproduced by this

GenericClass: class <T> {
    value: T
    init: func (=value)
}

test1 := GenericClass<Int> new(1)
test2 := GenericClass<Int> new(2)

printf("%d\n", test1 value)
printf("%d\n", test2 value)

@alexnask
Copy link

I think this is because alloca is used to initialize the memory of the generic value (here named value) inside the class's defaults, so it just gets free'd immediately...

void test__GenericClass___defaults___impl(test__GenericClass* this) {
    types__Class___defaults___impl((types__Class*) this);
    this->value = Memory__alloca(this->T->size);
}

Probably should malloc and have some notion of a free function in the language, which would have calls to free for the generic values.

@alexnask
Copy link

I think a good solution would be to auto-generate free functions that do cleanup like generic values or a Closure's context etc. and then call the user defined __destroy__ function (which I think was originally going to be a finalizer but was never implemented).

@horasal
Copy link
Author

horasal commented Mar 12, 2016

@Shamanas
Good idea,
It sounds good to add cleanup sections for every block (functiondecl, classdecl, etc.) to free the generic/closure. I will try to implement it.

@thomasfanell
This reminds me that ooc-kean uses the __onheap__ property to allocate the memory on the heap. So I guess you may want to add a __onheap__ before T?

GenericClass: class <T> {
    value: __onheap__ T
    init: func (=value)
}

arrayOfGenericClass := GenericClass<Int>[5] new()
// Initialize array elements
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] = GenericClass<Int> new(i + 1)
for (i in 0 .. arrayOfGenericClass length)
    "%d" printfln(arrayOfGenericClass[i] value)
for (i in 0 .. arrayOfGenericClass length)
    arrayOfGenericClass[i] free()
arrayOfGenericClass free()

@alexnask
Copy link

@zhaihj While I do think it's the way to go, I think it should happen in another issue/PR and everyone needs to agree with the way it will be done, since it will be a breaking change.

The way I see it, every cover/class decl gets an autogenerated free function and the current free functions are just renamed to __destroy__ (still calling free on their children etc.)

EDIT: rock could also detect if a free function is already manually declared and not generate one itself, that way a lot of code is backwards compatible.

EDIT2: I may fork a version of rock with radical changes, perhaps give ownership and some form of light-RAII a shot, I always wondered what ooc would look like with some of those features and GC off.

@thomasfanell
Copy link

@zhaihj Conflicts

@horasal
Copy link
Author

horasal commented Mar 14, 2016

rebase to master

@horasal
Copy link
Author

horasal commented Mar 17, 2016

Oops, it seems that I merged an un-related branch.
rebased to master again

@thomasfanell
Copy link

Merged via #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants