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

Remove delegates (RFC 31) #1534

Merged
merged 1 commit into from
Jan 27, 2017
Merged

Remove delegates (RFC 31) #1534

merged 1 commit into from
Jan 27, 2017

Conversation

Theodus
Copy link
Contributor

@Theodus Theodus commented Jan 26, 2017

This PR removes delegates as a language feature by removing the implementation and tests for delegates from the compiler.

Closes #1514

Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've missed some tests about delegates in parse_expr.cc.

I've also left a comment on the source code.

In general it looks good and could be merged as is but I think it needs some more polishing to be considered a "true" removal of the feature.

// Order should be:
// id type value delegate_type
REORDER(0, 1, 3, 2);
AST_NODE(TK_NONE); // provides
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That node is unnecessary. I think we can completely remove that child node and refactor the parts of the compiler that try to access it, like we discussed in #1514.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the node and calls to it result in the SugarTest.ObjectWithField test failing with the following message:

Error:
Unexpected child node found, flet
    object let x: T = 3 fun foo() => 4 end
           ^
test/libponyc/util.cc:271: Failure
Value of: false
  Actual: false
Expected: true
test/libponyc/util.cc:317: Failure
Expected: check_ast_same(expect_package, package) doesn't generate new fatal failures in the current thread.
  Actual: it does.
test/libponyc/sugar.cc:1531: Failure
Expected: test_equiv(short_form, "sugar", full_form, "parse") doesn't generate new fatal failures in the current thread.
  Actual: it does.

Copy link
Member

@Praetonus Praetonus Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing the blank provides node added to fields by the object literal sugar will fix the problem. It's in pass/sugar.c line 840.

@Theodus
Copy link
Contributor Author

Theodus commented Jan 26, 2017

@Praetonus The provides node has been removed from the field AST.


case TK_EMBED:
{
if(ast_childidx(ast, 3) == NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this isn't the right thing to do. With the change to the grammar, embed nodes will always have 3 childs and this condition will always be true, which means we'll never check that an embedded field has a valid type.

We should always go through the checks and return AST_OK instead of flatten_provides_list at the end.

@@ -279,10 +279,12 @@ ast_result_t pass_flatten(ast_t** astp, pass_opt_t* options)

case TK_FVAR:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TK_FVAR and TK_FLET cases should be completely removed here, or else they'll go through the same checks as TK_EMBED, which is wrong.

@Theodus Theodus added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Jan 27, 2017
Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@jemc jemc merged commit a2bd75a into ponylang:master Jan 27, 2017
ponylang-main added a commit that referenced this pull request Jan 27, 2017
@Theodus Theodus deleted the remove-delegates branch January 27, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants