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

JAMM-56: Fix access issue to public fields of package private classes #57

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

blerer
Copy link
Collaborator

@blerer blerer commented Jun 23, 2023

No description provided.

Copy link
Collaborator

@ekaterinadimitrova2 ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

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

Tests look good, this seems correct. Now we count more but correct

"packageProtectedClassPrivateField");
}

public long measure(MemoryMeter meter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So technically, isn't this again measureDeep? I would call it that probably as otherwise, the test where we compare measure with measureDeep reads a bit weird

this.privateField = privateField;
}

public long measure(MemoryMeter meter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be called again measureDeep

Comment on lines 37 to 44
return meter.measure(this)
+ meter.measureDeep(publicField)
+ meter.measureDeep(packageProtectedField)
+ meter.measureDeep(protectedField)
+ meter.measureDeep(privateField)
+ packageProtectedClass.measure(meter);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks ok to me, just left a small comment about naming.
CI pushed here:
https://app.circleci.com/pipelines/github/ekaterinadimitrova2/jamm/41/workflows/87ba096d-3d3f-404e-8d5e-5760ae84ef5e

Choose a reason for hiding this comment

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

Java 11 tests seemed to detect some size changes.

Copy link
Collaborator

@ekaterinadimitrova2 ekaterinadimitrova2 Jun 23, 2023

Choose a reason for hiding this comment

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

Thanks for noting, I guess you mean the failing tests? Those were failing even before due to some issues with @contended. More information can be found in the Readme

@blerer blerer merged commit 49c3e69 into jbellis:master Jun 23, 2023
@blerer blerer deleted the JAMM-56 branch June 23, 2023 16:01
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.

java.lang.IllegalAccessException on java.text.DigitList and com.google.protobuf.MapEntry$Metadata
3 participants