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

tracing of option and range types #341

Merged
merged 2 commits into from
May 10, 2016

Conversation

kikofernandez
Copy link
Contributor

This pull requests adds the tracing function and code generation for tracing option types. There is no possibility to add tests to check this runtime property, so the C generated code needs to be reviewed manually.

@kikofernandez kikofernandez changed the title tracing of option types tracing of option and range types Mar 9, 2016
@kikofernandez
Copy link
Contributor Author

the second commit adds code generation and runtime support for tracing Range types

@EliasC
Copy link
Contributor

EliasC commented Apr 6, 2016

@kikofernandez: Could you resolve these conflicts?

@kikofernandez
Copy link
Contributor Author

I can rebase and someone can take it from there. I am not sure that I have covered all the cases. Does that sound good?

@EliasC
Copy link
Contributor

EliasC commented Apr 6, 2016

That's what the hackathon is for! I can fix it then if no one else wants it.

@kikofernandez
Copy link
Contributor Author

pushed!

@kikofernandez
Copy link
Contributor Author

maybe it's a good idea if someone can review this PR before the hackathon then!

@PhucVH888
Copy link
Contributor

@kikofernandez : There are conflicts that must be resolved, could you resolve it as @albertnetymk and I will review it soon?

@kikofernandez
Copy link
Contributor Author

Not today

@albertnetymk
Copy link
Contributor

@TheGrandmother 's vat is failing possibly due to missing the tracing in this PR.

@kikofernandez
Copy link
Contributor Author

I have rebased this PR and checked that the generated code from tests/encore/basic/maybeTypes.enc is correct. I think this is ready for another review! :)

@EliasC
Copy link
Contributor

EliasC commented May 10, 2016

I think this looks good! I had some ideas for improvement (see below), but they will not work. I'll merge this in a little while.

Here's the (broken) idea:

A thought for Maybe types (that could be applicable to other types as well). I would think that we will always statically know the type of the "contents" of an instance of a Maybe, and so it would be nice to not have to carry with us the type dynamically. Would it be possible to inline what option_trace is currently doing? So when tracing a value of type Maybe Foo, instead of calling

pony_traceobject(_ctx, some_option, option_trace);

and relying on some_option to know it's own dynamic type, we would do something like

pony_trace(some_option);
if(some_option->tag != NOTHING) {
  pony_traceobject(_ctx, some_option->val, _enc__trace_Foo);
}

Nested Maybes would result in nested if-statements. This way, we could save a pointer per Maybe instance.

Here is why it won't work:

There are times when we want to trace a Maybe without knowing that it is a Maybe. For example, consider the following class

passive class Foo<t>
  f : t

In the trace function for Foo we need to dynamically find the concrete type of t, and since this could be a Maybe, (e.g. an instance of the class Foo<Maybe int>), we need to be able to dynamically find the type of the contents of this (i.e. what trace_option currently does).

@EliasC EliasC merged commit f09fc06 into parapluu:development May 10, 2016
@kikofernandez kikofernandez deleted the fix/option-types branch May 12, 2016 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants