-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8368966: Remove spurious VMStructs friends #27583
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back fandreuzzi! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
|
||
template <typename E> | ||
class GrowableArray : public GrowableArrayWithAllocator<E, GrowableArray<E>> { | ||
friend class VMStructs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit surprised to see that you added a friend declaration. Was this done because you removed the one in the parent class?
I see that vmStructs.cpp has GrowableArray listed:
nonstatic_field(GrowableArrayBase, _len, int) \
nonstatic_field(GrowableArrayBase, _capacity, int) \
nonstatic_field(GrowableArray<int>, _data, int*)
So, I guess it makes sense to move it here. OTOH, the exposed _data
field is inside GrowableArrayView, so maybe it would work to put the friend declaration there instead?
I guess either way is fine but there's a risk that there will be some churn about where to put the friend declaration if someone wants add one of the classes between (and including) GrowableArrayView and GrowableArray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this done because you removed the one in the parent class?
Yeah that was my reasoning.
I guess either way is fine but there's a risk that there will be some churn about where to put the friend declaration if someone wants add one of the classes between (and including) GrowableArrayView and GrowableArray.
To me it looks less confusing to have the friend
declaration for the type which is referenced in vmStructs
. But both are legal, do you think moving the friend
declaration to the type which declares the referenced field would be more appropriate? If yes, I'll track other similar patterns too.
In this PR I propose a small clean up to remove several spurious friends of
VMStructs
. Either I could not find any reference to them invmStructs*
, or no private symbols is mentioned.Passes tier1 and tier2 (fastdebug).
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27583/head:pull/27583
$ git checkout pull/27583
Update a local copy of the PR:
$ git checkout pull/27583
$ git pull https://git.openjdk.org/jdk.git pull/27583/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27583
View PR using the GUI difftool:
$ git pr show -t 27583
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27583.diff
Using Webrev
Link to Webrev Comment