-
Notifications
You must be signed in to change notification settings - Fork 69
For synthetic $default methods, lookup the annotations on their corresponding non-synthetic method #85
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
Conversation
/** | ||
* For synthetic $default methods, pull the annotations from the corresponding method | ||
*/ | ||
val alternateDefaultSignature = mVisibility?.name?.let { className -> |
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.
Would it be okay for you to take an extra step and consolidate your approach with the hackish filtering of publishedApiSignatures
few lines below?
It seems like the handling is mostly identical and we have two similar codepaths, e.g. isPublishedApiWithDefaultArguments
and alternateDefaultSignature
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'll take a look!
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.
Refactor is pushed. It took a bunch of changes. I'm pretty sure this is sub-optimal in terms of performance (the $annotations
method could potentially be looked up twice for field + getter). Let me know how much of an issue it is.
As a nice side effect, I could enable all of the "inline" tests
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.
Something else I wanted to mention: the new changes now collect both visible + invisible annotations for markers (because @PublishedApi
is invisible it seems). I'm not super versed in the visible vs invisible difference but this too might have a performance hit?
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.
Performance doesn't seem to be an issue here; thanks for expressing your concerns though!
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.
Superb, thanks! 👍
…ns on their corresponding non-synthetic method Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85
…ns on their corresponding non-synthetic method Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85
…ns on their corresponding non-synthetic method Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85
…ns on their corresponding non-synthetic method Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85 Moved from Kotlin/binary-compatibility-validator@66031d9
…ns on their corresponding non-synthetic method Fixes Kotlin/binary-compatibility-validator#58 Pull request Kotlin/binary-compatibility-validator#85 Moved from Kotlin/binary-compatibility-validator@66031d9
Follow up from #81 as the logic is pretty similar
closes #58
PS: not sure why I disabled maintainer edits on that other PR but this one has the checkbox enabled ✅ so feel free to push here if needed