AndroidEntryPointFinder added #37
Conversation
There was a problem hiding this comment.
Are you planning to add support for the androidx/support library version of Activity etc? e.g. https://developer.android.com/reference/androidx/activity/package-summary or https://developer.android.com/reference/android/support/packages
| val eps = ArrayBuffer.empty[Method] | ||
|
|
||
| var peps: List[String] = List("onCreate", "onRestart", "onStart", "onResume", "onStop", "onDestroy", |
There was a problem hiding this comment.
Do not use a 'var', use individual 'val's defined outside of the method.
| val mc = sc.findMethod(pep) | ||
| for (m ← mc) { | ||
| if (m.body != null) { |
There was a problem hiding this comment.
m.body should never be null, it's an Option, thus either using isDefined or just mapping over it as part of the 'for' should work
| classHierarchy.foreachSubclass(ObjectType("android/app/Activity"), project) { sc ⇒ | ||
| for (pep ← peps) { | ||
| val mc = sc.findMethod(pep) |
There was a problem hiding this comment.
Maybe better use Scala's extended 'for' syntax to have just one 'for' that iterates over 'peps', 'mc' and possibly even 'm.body' (see next comment).
| var peps: List[String] = List("onCreate", "onRestart", "onStart", "onResume", "onStop", "onDestroy", | ||
| "onActivityResult") | ||
| classHierarchy.foreachSubclass(ObjectType("android/app/Activity"), project) { sc ⇒ |
There was a problem hiding this comment.
Are you aware that 'foreachSubclass' a) is not reflexive (i.e., it won't include the class file for the Activity class here) and b) may include a subclass multiple times if the given ObjectType is an interface (Activity isn't, but I didn't check your other invocations of this method)?
| } | ||
|
|
||
| peps = List("onCreate", "onStartCommand", "onBind", "onStart") |
There was a problem hiding this comment.
Code is utterly repetitive, please extract a method to do this.
|
|
||
| peps = List("onLocationChanged", "onProviderDisabled", "onProviderEnabled", "onStatusChanged") | ||
| classHierarchy.foreachSubtype(ObjectType("android/location/LocationListener")) { st ⇒ |
There was a problem hiding this comment.
Why are you using foreachSubtype here, but foreachSubclass on the previous ones? foreachSubclass directly provides you with the classfile, already ensuring that it is available.
| @@ -355,71 +355,47 @@ object AllEntryPointsFinder extends EntryPointFinder { | |||
| * @author Tom Nikisch | |||
| */ | |||
| object AndroidEntryPointsFinder extends EntryPointFinder { | |||
|
|
|||
| val ActivityEPS: List[String] = List("onCreate", "onRestart", "onStart", "onResume", "onStop", "onDestroy", | |||
There was a problem hiding this comment.
Use camel case with lower case first letter here.
| } | ||
|
|
||
| def verifyEPS(possibleEPS: List[String], sc: ClassFile): ArrayBuffer[Method] = { |
There was a problem hiding this comment.
Can you move the foreachSubclass invokation to this method, too? You can just pass List("onNmeaMessage") for the last one (or, e.g. use Traversable[String] for the parameter type).
There was a problem hiding this comment.
Why is this called "verifyEPS"? What verification does it perform?
| def verifyEPS(possibleEPS: List[String], sc: ClassFile): ArrayBuffer[Method] = { | ||
| val eps = ArrayBuffer.empty[Method] | ||
| for (pep ← possibleEPS; m ← sc.findMethod(pep) if m.body.isDefined && !eps.contains(m)) { |
There was a problem hiding this comment.
Depending on how large eps grows, it may be more (or less) performant to use a Set (where contains is constant time instead of linear, but addition may be more costly).
| val eps = ArrayBuffer.empty[Method] | ||
| for (pep ← possibleEPS; m ← sc.findMethod(pep) if m.body.isDefined && !eps.contains(m)) { | ||
| eps.append(m) |
There was a problem hiding this comment.
You may want to use += instead of append (and ++= instead of appendAll).
| */ | ||
| object AndroidEntryPointsFinder extends EntryPointFinder { | ||
|
|
||
| val activityEPS: List[String] = List("android/app/Activity", "onCreate", "onRestart", "onStart", "onResume", |
There was a problem hiding this comment.
I'm not a fan of Lists in which certain indexes have different semantics, but if @errt says it's fine I don't mind.
There was a problem hiding this comment.
Ah, didn't notice that. Yes, please have the list only for the methods and provide the ObjectTypes individually.
| * | ||
| * @author Tom Nikisch | ||
| */ | ||
| object AndroidEntryPointsFinder extends EntryPointFinder { |
There was a problem hiding this comment.
Since you didn't answer:
Are you planning to add support for the androidx/support library version of Activity etc? e.g. https://developer.android.com/reference/androidx/activity/package-summary or https://developer.android.com/reference/android/support/packages
If you're not doing that, please at least add a comment that makes this clear.
There was a problem hiding this comment.
Currently i am not planning to do that but i can probably do it later. Thank you for your suggestion.
|
|
||
| override def collectEntryPoints(project: SomeProject): Traversable[Method] = { | ||
| val eps = ArrayBuffer.empty[Method] | ||
| for (key ← defaultEPS.keys) { |
There was a problem hiding this comment.
Last change, then I guess I'll accept it:
Please use for((k,v) <- map) syntax here and use meaningful names for k and v.
You then obviously don't need to query the map for the key as you got the value already in v.
No description provided.