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

::= syntax in covers #782

Closed
davidhesselbom opened this Issue Jul 29, 2014 · 19 comments

Comments

Projects
None yet
4 participants
@davidhesselbom
Contributor

davidhesselbom commented Jul 29, 2014

Why can't covers use the ::= syntax?

import math

// Does not build
PointCoverA : cover {
    x, y: Float
    Norm ::= (this x pow(2.0f) + this y pow(2.0f)) sqrt()
}

// Builds just fine
PointCoverB: cover {
    x, y: Float
    Norm: Float { get { (this x pow(2.0f) + this y pow(2.0f)) sqrt() } }
}

// Builds just fine
PointClassA: class {
    x, y: Float
    Norm ::= (this x pow(2.0f) + this y pow(2.0f)) sqrt()
}

// Builds just fine
PointClassB: class {
    x, y: Float
    Norm: Float { get { (this x pow(2.0f) + this y pow(2.0f)) sqrt() } }
}
@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Jul 29, 2014

Runnable testcase:

import math

// Does not build
Point: cover {
    x, y: Float
    norm ::= (this x pow(2.0f) + this y pow(2.0f)) sqrt()
}

main: func {
    p := (2, 2) as Point
    "norm of (2, 2) = #{p norm}" println()
}

(Also, style note to @davidhesselbom - methods & getters should be lowercase in ooc - has nothing to do with the error but since I see that as a recurring pattern in your bug reports I thought I'd mention it).

Here's clang's error report:


rock_tmp/ooc/issue782/issue782.c:25:66: error: member reference type 'issue782__Point *' (aka 'struct _issue782__Point *') is a pointer; maybe you meant to use '->'?
    return lang_Numbers__Float_sqrt((lang_Numbers__Float_pow(this.x, 2.0f) + lang_Numbers__Float_pow(this.y, 2.0f)));
                                                             ~~~~^
                                                                 ->
rock_tmp/ooc/issue782/issue782.c:25:106: error: member reference type 'issue782__Point *' (aka 'struct _issue782__Point *') is a pointer; maybe you meant to use '->'?
    return lang_Numbers__Float_sqrt((lang_Numbers__Float_pow(this.x, 2.0f) + lang_Numbers__Float_pow(this.y, 2.0f)));
                                                                                                     ~~~~^
                                                                                                         ->

It's true that ::= was designed mainly with classes in mind, not covers, so there might be a few bugs like this one lurking around. I'm guessing this one is middle-end related.

zhaihj added a commit to zhaihj/rock that referenced this issue Jul 30, 2014

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Jul 30, 2014

Sorry for noise, that commit does not work ...

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Jul 30, 2014

@zhaihj It doesn't look too bad though - how exactly does it not work?

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Jul 30, 2014

@fasterthanlime
Directly set isThisRef in onPropertyDeclGetterStart does make the above testcase compliable.
But it seems that stack pop isn't always able to catch CoverDecl

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Jul 30, 2014

@fasterthanlime thanks for the style note, I appreciate it. We're converting a C# library into ooc, and we were toying with the idea of letting capitalization indicate which access modifier is used in the original code, hoping we would eventually be able to hack Rock to use these caps to insert actual access modifiers somewhere along the way. I'll keep it lowercase in bug reports from now on.

fasterthanlime added a commit that referenced this issue Aug 15, 2014

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Aug 15, 2014

Simply a matter of resolution order in properties. Fixed now, enjoy :)

@fasterthanlime fasterthanlime added the Bug label Aug 15, 2014

@fasterthanlime fasterthanlime added this to the 0.9.9 milestone Aug 15, 2014

@fasterthanlime fasterthanlime self-assigned this Aug 15, 2014

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Aug 6, 2015

Can this be made to work for static properties as well? Or does it already? What would be the syntax, then?

property: static := 1

???

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Aug 7, 2015

I would expect the syntax to be

// The "static expr" is kinda funky but it actually works and is idiomatic in ooc
prop ::= static 1

(If it does work, that is)

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

I know this was closed long ago, but I'm wondering about something related, and I don't think I should open a new issue about it. Here goes:

Can covers have set properties? Specifically, can those set properties set values in covers that they are a cover from?

Vector: cover {
    x, y: Int
    init: func
}
Size: cover from Vector extends Vector {
    init: func
    width: Int {
        get { this x }
        set(value) { this x = value }
    }
    height: Int {
        get { this y }
        set(value) { this y = value }
    }
}

This doesn't build, because

error: lvalue required as left operand of assignment
         set(value) { this x = value }

and I'm not even sure what that means. The get property seems to work fine, though:

Vector: cover {
    x, y: Int
    init: func
}
Size: cover from Vector extends Vector {
    init: func
    width: Int { get { this x } }
    height: Int { get { this y } }
}

v := Vector new()
v x = 1
v y = 2
s := v as Size
s width toString() println() // prints 1, yay
s height toString() println() // prints 2, yay

Am I just missing an @ somewhere?

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

I hadn't thought about

    width: extern (x) Int
    height: extern (y) Int

which works, and which is obviously much, much better than

    width: Int {
        get { this x }
        set(value) { this x = value }
    }
    height: Int {
        get { this y }
        set(value) { this y = value }
    }

Thanks for watching, though.

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Nov 17, 2015

@davidhesselbom actually that's an interesting bug

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

What is? The build error?

Btw, do you still need the extern keyword, or is there another syntax I should use when the cover I'm a cover from isn't... external?

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Nov 17, 2015

@davidhesselbom your am I missing a @ somewhere comment made me think that this bit of code would fail:

Foo: cover {
    bar: Int

    baz: Int {
        set (baz) { this bar = baz }
    }
}

f: Foo
f bar = 1
f baz = 2

if (f bar != 2) {
    raise("expected f bar == 2")
}

..but it actually works so it must be linked to the extends

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Nov 17, 2015

Whereas this doesn't work:

UnderFoo: cover {
    bar: Int
}

Foo: cover from UnderFoo extends UnderFoo {
    baz: Int {
        set (baz) { this bar = baz }
    }
}

f: Foo
f bar = 1
f baz = 2

if (f bar != 2) {
    raise("expected f bar == 2")
}
@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

Does it fail the test at the end, or does it simply not build?

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Nov 17, 2015

@davidhesselbom I opened #949 to track it, details are there — but I won't have time to close it in the near future

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

Ok, thanks though.

I'll ask again about the other thing: Is there an alternative to width: extern (x) Int when creating a cover from a cover that doesn't come from an external library, or is extern simply the keyword for "declared in another cover, somewhere, but not here"?

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Nov 17, 2015

@davidhesselbom no alternative that I know of. an extern name is the underlying name of a field declared somewhere else

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 17, 2015

Okay, just making sure. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment