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

Fix #204 — calling methods on null receiver does not crash #482

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

TobiasWrigstad
Copy link
Contributor

This PR adds an additional branch before each method call or message send in Encore. Basically

x.m() 

is turned into (mock C code):

if (x) { x.m(); } else { fprintf(stderr, "error message"); abort() }

An error message looks like this

Error: empty receiver in x ! test(...) in "src/tests/encore/basic/recvNullSend.enc" (line 8, column 9)

using ! or . appropriately.

The reason to add the branch at the call-site rather than at the beginning of each method was because;

  1. non-null types will be able to avoid this check, but this cannot be done inside the method
  2. the C compiler will be able to elide redundant checks and be smart about optimising

Note that the tests are written with PR 481 in mind. Without this PR, there is no support for testing this PR.

@albertnetymk
Copy link
Contributor

I thinking moving the macro in Header.hs to encore.h would make it more readable.

@TobiasWrigstad
Copy link
Contributor Author

TobiasWrigstad commented Jun 13, 2016

@albertnetymk DONE

@TobiasWrigstad TobiasWrigstad force-pushed the issue_204 branch 2 times, most recently from 7e1fef4 to d3e5f18 Compare June 13, 2016 09:01
@TobiasWrigstad
Copy link
Contributor Author

@albertnetymk bump!

@albertnetymk
Copy link
Contributor

This PR is could be merged after #481.

Some nitpicking:

  • targetNullCheck is defined almost identically in two places, which could be leveled up so that it could be used in both clauses. Sth like:
targetNullCheck "." ntarget target name meta = ...
targetNullCheck "!" ntarget target name meta = ...
  • this PR exceeds 80 line width in some places

@TobiasWrigstad
Copy link
Contributor Author

@albertnetymk I hope I have addressed both points now.

@TobiasWrigstad TobiasWrigstad changed the title Closing issue #204 Fix #204 — calling methods on null receiver does not crash Jun 15, 2016
@albertnetymk
Copy link
Contributor

Looks great. Can be merged after #481

@TobiasWrigstad
Copy link
Contributor Author

@albertnetymk FYI #481 is now merged.

@albertnetymk
Copy link
Contributor

The two added tests fail for me, sth like:

     | ERROR: test encore/basic/recvNullCall failed with output:
     | vvv OUTPUT vvvvvvvvvvvvvvvvv
     | Error: empty receiver in x.test(...) in "recvNullCall.enc" (line 8, column 7)
     | vvv EXPECTED vvvvvvvvvvvvvvv
     | Error: empty receiver in x.test(...) in "src/tests/encore/basic/recvNullCall.enc" (line 8, column 7)

@TobiasWrigstad
Copy link
Contributor Author

And to be sure — did you update to support #481 first?

@albertnetymk
Copy link
Contributor

Think so. Just need to pull dev, nothing else, right?

@TobiasWrigstad
Copy link
Contributor Author

I'll investigate. The programs are expected to fail but that should be counted as passing tests.

@TobiasWrigstad
Copy link
Contributor Author

I believe something may have changed in the mean time causing the error output of the failing tests to change, which made the .err file not match. Fixed now IMO. Please try again.

@TobiasWrigstad
Copy link
Contributor Author

@albertnetymk bump

@albertnetymk
Copy link
Contributor

Sorry for the delay. dev was broken when I tried this PR this morning.

Tried the HEAD~ with this PR, and it works OK. Merging now.

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.

2 participants