Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

wip: fix: keep implicit modifiers in reflective proxies #4075

Conversation

I-Al-Istannen
Copy link
Collaborator

@I-Al-Istannen I-Al-Istannen commented Aug 3, 2021

Blocked

This PR is blocked by #4205 as it does the same thing.

Problem

Spoon adds implicit public abstract modifiers to methods (as they are present in the MethodBinding) but explicitly ignores them in the reflective model. This is IMHO quite unexpected and should be fixed.

Solution in this PR

This PR just removes the checks added by #1914. I couldn't really find any justification for ignoring them but maybe the MethodBinding was different back then. They were introduced in commit 2fc84bb if you want to have a look.

We can not mark the modifiers as implicit as far as I can tell, as reflection doesn't really tell us whether it was present in the source code or added implicitly.

@I-Al-Istannen I-Al-Istannen changed the title wip: fix: implicit modifiers reflective proxy wip: fix: keep implicit modifiers in reflective proxies Aug 3, 2021
@I-Al-Istannen I-Al-Istannen force-pushed the fix/implicit-modifiers-reflective-proxy branch from 6316565 to b99bf57 Compare October 6, 2021 17:45
@I-Al-Istannen
Copy link
Collaborator Author

Let's hold a minute of silence for this PR that was open for two months but is now completely contained in #4205.
grafik

@I-Al-Istannen I-Al-Istannen deleted the fix/implicit-modifiers-reflective-proxy branch October 7, 2021 08:26
@monperrus
Copy link
Collaborator

monperrus commented Oct 7, 2021

done, and to stay on the positive side, let's celebrate that #4205 is merged.

4998390

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants