Inline call to BundleHelper.getPlatformAdmin().getFactory()#1716
Inline call to BundleHelper.getPlatformAdmin().getFactory()#1716HannesWell merged 1 commit intoeclipse-pde:masterfrom
Conversation
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
Given that BundleHelper.getPlatformAdmin().getFactory() is already cached and we only have three call sites I wonder if it would not be better to replace calls directly with
BundleHelper.getPlatformAdmin().getFactory()
as in the suggestions.
|
|
||
| protected MinimalState(MinimalState state) { | ||
| this.fState = stateObjectFactory.createState(state.fState); | ||
| this.fState = SingletonHolder.STATE_OBJECT_FACTORY.createState(state.fState); |
There was a problem hiding this comment.
| this.fState = SingletonHolder.STATE_OBJECT_FACTORY.createState(state.fState); | |
| this.factory = BundleHelper.getPlatformAdmin().getFactory(); | |
| this.fState = factory.createState(state.fState); |
As a result of 14f66ae, it might happen that the MinimalState is shut down, even though it was never started. In such a case, the class is implicitly loaded while stopping the plug-in. When loading this class, the "stateObjectFactory" is initialized using the PlatformAdmin provided by the PDE Build bundle. If this bundle is stopped before the PDE Core bundle, an exception is thrown because the PlatformAdmin is no longer available. The BundleHelper is already cached by the PDE Build plug-in and can therefore be called directly, making the "stateObjectFactory" field obsolete. Closes eclipse-pde#1714
ptziegler
changed the title
That makes sense. The singleton in the BundleHelper is implicitly synchronized by the I've applied your changes and updated the commit title/description. |
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
Looks good to me, factory can become a (protected) instance field then we even have only one reference at all (but not a static one), but even with this for I don't think we will see any issues.
Only "addBundle" is something that might be called more often here.
|
The four new compiler warnings are about the parameters of the RequiredPackageVersionRange record, which is unrelated to this PR and from my perspective also wrong. |
I get the same warning in the IDE, seems an issue of JDT, should we keep this PR as a reference for now? |
|
@srikanth-sankaran can you probably have a look? Looks like all constructor parameter of a record are not marked as "unused". |
That seems to be already fixed via eclipse-jdt/eclipse.jdt.core#3905. So I think it's save to merge the PR? |
If there are no other pending actions, yes. |
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the fix and I'm glad that the review lead to an even simpler solution. :)
4 participants
As a result of 14f66ae, it might happen
that the MinimalState is shut down, even though it was never started. In
such a case, the class is implicitly loaded while stopping the plug-in.
When loading this class, the "stateObjectFactory" is initialized using
the PlatformAdmin provided by the PDE Build bundle. If this bundle is
stopped before the PDE Core bundle, an exception is thrown because the
PlatformAdmin is no longer available.
The BundleHelper is already cached by the PDE Build plug-in and can
therefore be called directly, making the "stateObjectFactory" field
obsolete.
Closes #1714