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

Preprocessor improvements #34

Merged
merged 6 commits into from
Jan 11, 2017
Merged

Preprocessor improvements #34

merged 6 commits into from
Jan 11, 2017

Conversation

area
Copy link
Member

@area area commented Jan 10, 2017

Depends on the new solution to #28, so not to be merged until that is.

Introduces an improvement to the preprocessor and instrumentation in
general. The preprocessor improvement is the fix to #32.

The instrumentation improvement derived from how I saw SolCover behaving
with statements immediately following comments.

Previously, we traversed the whole AST, and when we encountered a
statement we checked if we could instrument it i.e. if it was
standalone. This introduced a bunch of cases that we wanted to check
(is it in a block? it is immediately after a block? etc.) and some edge
cases that didn’t work properly (comments).

Instead, we now stop traversing the AST in situations where we know we
can’t instrument, and assume that if we find a statement that we can
instrument it. We can’t instrument f(x) easily (see below) in
these situations, so we don’t traverse them:

  • if (f(x))
  • arr[f(x)
  • var x = f(x)
  • g(f(x))

Of course, going forward we might ‘decide’ that we can in fact
instrument these. e.g. arr[f(x)] could be replaced by

var _some_unique_id = f(x);
arr[_some_unique_id];

and then instrumented as appropriate.

@area
Copy link
Member Author

area commented Jan 10, 2017

Those tests all pass locally for me, so I'm not sure what's going on here....

@cgewecke
Copy link
Collaborator

@area the failing tests on CI are because of this PR was just merged at solidity-parser. I pegged SP to master because it had bug fixes that weren't published to npm but that's obviously a bad idea. I think just getting SP off master in the package will run everything clear for this PR. And i'll open another branch right now that makes the changes necessary for SP #57 to work with solcover.

@area
Copy link
Member Author

area commented Jan 10, 2017

Ah, okay. I'm fine with pinning to master on solidity-parser, but it should probably be a specific commit / tag.

@cgewecke
Copy link
Collaborator

@area Sorry - missed that last comment. I just opened a PR for solcover master that is a temporary patch for the SP problem. I just used the result of npm install solidity-parser which is ^0.2.0. I'll push a change here now but pinned to 0.2.0.

@cgewecke
Copy link
Collaborator

Highlighting is perfect from what I can see. 👍 Really good. I'm looking at instanbul and solcover reports side by side for a bunch of if / else / loop code and solcover is actually slightly more accurate.

@area
Copy link
Member Author

area commented Jan 11, 2017

Rebased, so you'll need to git pull --rebase if you want to work on this branch.

area and others added 6 commits January 11, 2017 14:41
Previously, we traversed the whole AST, and when we encountered a
statement we checked if we could instrument it i.e. if it was
standalone. This introduced a bunch of cases that we wanted to check
(is it in a block? it is immediately after a block? etc.) and some edge
cases that didn’t work properly (comments).

Instead, this stops traversing the AST in situations where we know we
can’t instrument. We can’t instrument `f(x)` easily (see below) in
these situations, so we don’t traverse them:

* `if (f(x))`
* `arr[f(x)`
*  `var x = f(x)`
* `g(f(x))`

Of course, going forward we might ‘decide’ that we *can* in fact
instrument these. e.g. `arr[f(x)]` could be replaced by

```
var _some_unique_id = f(x);
arr[_some_unique_id];
```
and then instrumented as appropriate.
@cgewecke
Copy link
Collaborator

@area I have nothing to add here. I'll happily write some unit tests for any instrumentSolidity.js lines that aren't getting codecov coverage when you merge this though. Looks really good.

@area area merged commit 9aa7693 into master Jan 11, 2017
@area area mentioned this pull request Jan 25, 2017
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