-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fixed generating wrong arguments in stream_mk() #396 #401
Conversation
Does |
|
@@ -346,7 +346,7 @@ methodImplStream cname m = | |||
this = (Ptr . AsType $ classTypeName cname, Var thisName) | |||
retVar = Var "_stream" | |||
declVar = Decl (stream, retVar) | |||
streamMk mtype = Call streamMkFn [AsExpr encoreCtxVar, runtimeType mtype] | |||
streamMk mtype = Call streamMkFn [AsExpr encoreCtxVar] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need to make the encoreCtxVar
an Expr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not pretty sure but it could be used that way. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare the C code for both your version and Kiko's. If Kiko's version make sense, compiles, and passes the tests, then use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kikofernandez : Where is your implementation and which branch should I checkout Kiko?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just try to remove AsExpr
and see if it compiles!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will commit an updated version. Ultimately, I am not so clear what are the differences with and without AsExpr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhucVH888 Did you look at the generated C? There you'll see the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@supercooldave There is not any difference.
Both versions (with or without AsExpr
) generate the same function stream_t* _stream = stream_mk(_ctx);
As I tested with this example:
class Foo
stream bar() : int {
yield 99;
yield 100;
}
class Main
def main() : void
print(get (new Foo).bar())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that there won't be a difference in the generated C. The different kind of C-expressions in CCode
(Name
, Lval
, Expr
, Stat
and TopLevel
) are there to give some extra sanity checking. For example, the lefthand side of an assignment must be an Lval
. Writing Haskell code that generates 42 = x
will not compile as 42
is an Expr
. However, this also means that we sometimes need to use AsExpr
when we want to use an Lval
where an Expr
is expected. In many cases (such as this one), this cast can be inferred by the type system)
Removed |
The pull request
stream.h
From
stream_t *stream_mk();
. tostream_t *stream_mk(pony_ctx_t *ctx);
.ClassDecl.hs
.Generate
stream_mk(ctx)
instead ofstream_mk(ctx,ENCORE_PRIMITIVE)
.make test
.