-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Clean up synthetic accessor method rot #2762
Conversation
c00d6d4
to
71c806a
Compare
Hah I pushed a branch that migrated to 6.0.0 recently as well, though with no thought given to refactoring in general: https://github.com/sjudd/glide/tree/pmd_6_0_0 |
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.
Looks better than my changes, I can try to rebase on top of it to get to 6.0.0+.
Just a couple of questions.
@@ -571,8 +574,9 @@ protected RequestOptions getMutableOptions() { | |||
return into(target, /*targetListener=*/ null); | |||
} | |||
|
|||
@RestrictTo(Scope.LIBRARY) |
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'm ok not using this for non-public methods. I assume anything that's non-public can be broken and even some of the public APIs we have are breakable (those really should be marked with @RestrictTo
or some equivalent.
Fine to keep it, just as a note for the future (or a discussion point).
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.
Lot of negation there :)
You're right: default
methods don't need this, if someone uses "internal" (name/keyword for default visibility in other languages) API, they can fix it if we break something. protected
on public classes may need this, and public
methods that are only public because of packages, and not because it's public API also need this. I'll remove from default
ones.
@SuppressWarnings("WeakerAccess") | ||
@Synthetic | ||
final DecodeHelper<R> decodeHelper = new DecodeHelper<>(); | ||
private final DecodeHelper<R> decodeHelper = new DecodeHelper<>(); |
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.
Nice! Thanks!
@@ -111,7 +111,7 @@ static String getBitmapString(int size) { | |||
static class KeyPool extends BaseKeyPool<Key> { | |||
|
|||
public Key get(int size) { | |||
Key result = get(); | |||
Key result = super.get(); |
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.
Out of curiosity, what does the super do?
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.
Open the file on master
, there's a warning on it. It clarifies which get
is being called: both BaseKeyPool
and SizeStrategy
have a get
name in scope, so the super clears it up which one. Actually let me check that again, because it's a static inner class.
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.
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.
…l) methods should already be protected because users need to explicitly put accessor code into Glide packages to call the methods.
@sjudd merged master into this, resolved conflict, removed |
Thanks! I'll add the files you suggest to the gitignore, no particular reason to exclude them I don't think. |
Description
Since PMD 5.5.4 there's a
AccessorMethodGeneration
rule indesign
category thanks to Jake's request. The PR removes 20 synthetic methods and prevents adding any new ones.In this PR:
@Synthetic
annotation for future ci build failuresAvoidDuplicateLiterals
and simplifiedUncommentedEmptyConstructor
maven-metadata.xml
downloads.In some files (
ActiveResources
,DecodeJob
,ViewTarget.OnAttachStateChangeListener
, ) I took a different approach than making everything visible. It looks like in those classes the inner classes had some field envy to the outer class, so I moved the method to the outer class so there's only one visible@Synthetic
entry needed. In some places I also protected the published methods with@RestrictTo(Scope.LIBRARY)
to flag abuse.Motivation and Context
Followup to #1556 (comment)
After this PR only
disklrucache
synthetic accessors (22) remain.To check count, run
@sjudd Can you please add
!.idea/inspectionProfiles/Project_Default.xml
to.gitignore
and commit that file so everyone sees the same warnings in AS? I wanted to addJava/J2ME issues/Synthetic accessor call
inspection to the config, but I noticed that file is not version controlled.