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

Stateroot-enabled GetBlock #2583

Merged
merged 5 commits into from
Jul 8, 2022
Merged

Stateroot-enabled GetBlock #2583

merged 5 commits into from
Jul 8, 2022

Conversation

roman-khimov
Copy link
Member

Turned out to be more of a compiler problem than interop one.

Regular methods need this, because it'll be packed into parameters, but
inlined ones should deal with it in inlining code itself because method
receiver will be some local (aliased) variable anyway.
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #2583 (251c9bd) into master (c356c14) will decrease coverage by 0.29%.
The diff coverage is 66.20%.

@@            Coverage Diff             @@
##           master    #2583      +/-   ##
==========================================
- Coverage   84.93%   84.63%   -0.30%     
==========================================
  Files         297      297              
  Lines       37304    37491     +187     
==========================================
+ Hits        31683    31730      +47     
- Misses       4260     4379     +119     
- Partials     1361     1382      +21     
Impacted Files Coverage Δ
pkg/network/server.go 73.99% <0.00%> (-0.22%) ⬇️
pkg/rpc/client/client.go 84.25% <0.00%> (-1.35%) ⬇️
pkg/rpc/server/server.go 76.28% <0.00%> (ø)
pkg/services/helpers/rpcbroadcaster/client.go 0.00% <0.00%> (ø)
pkg/services/oracle/broadcaster/oracle.go 48.27% <0.00%> (ø)
pkg/wallet/wallet.go 84.46% <0.00%> (-0.98%) ⬇️
pkg/services/helpers/rpcbroadcaster/broadcaster.go 30.95% <10.52%> (-14.89%) ⬇️
pkg/services/notary/notary.go 82.58% <29.41%> (-0.70%) ⬇️
cli/wallet/legacy.go 78.64% <35.29%> (-11.25%) ⬇️
pkg/services/stateroot/validators.go 69.81% <45.45%> (-3.66%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ba9017...251c9bd. Read the comment docs.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Ugh, I'm not able to properly review the compiler-related code...

@@ -915,6 +915,9 @@ func (c *codegen) Visit(node ast.Node) ast.Visitor {
}
case *ast.SelectorExpr:
name, isMethod := c.getFuncNameFromSelector(fun)
if isMethod && name[0] == '*' {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this branch is not covered.
Also, usage analysis should have similar checks, so can we move this if inside of getFuncNameFromSelector?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me think about it, the branch itself should be covered by the TestInlinedMethod (it uses pointer receiver).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's ironic that this is the only place where we do care about isMethod, but probably it makes sense to return stripped name anyway, shouldn't affect current users in any way and any other method-aware code will probably need that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, we use the type of a local variable, not of a function formal receiver. I think using z = &inline.T{} should cover this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that works, added another test and reorder commits.

Notice that this doesn't differentiate between (*T) and (T) receivers always
treating them as is. But we have the same problem with arguments now and the
number of inlined calls is limited, usually we want this behavior.
c.funcs contains function names using base types, while methods can be defined
on pointers and the value returned from c.getFuncNameFromSelector will have an
asterisk. We can't have the same name used for (*T) and (T) methods, so just
stripping the asterisk allows to get the right one.
And add compiler/interop support for this.
@roman-khimov roman-khimov merged commit 34ddc99 into master Jul 8, 2022
@roman-khimov roman-khimov deleted the getblock-sr branch July 8, 2022 07:00
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.

3 participants