Skip to content
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

Fix: fix pretty-printing of imports for static methods #3716

Merged
merged 7 commits into from
Dec 9, 2020

Conversation

danglotb
Copy link
Member

@danglotb danglotb commented Dec 2, 2020

Hello,

I'm opening this pull-request that contains a test to reproduce the bug reported in #3618 and #3214.

Thank you

@monperrus
Copy link
Collaborator

Thanks @danglotb ! Are you trying to fix it?

@danglotb
Copy link
Member Author

danglotb commented Dec 3, 2020

Hi @monperrus, I'll investigate how to. Does the test seem correct? Do you think I can use it as a base to fix the issue?

@danglotb
Copy link
Member Author

danglotb commented Dec 4, 2020

Hi, @monperrus. Here my attempt. This pull-request closes #3214 and maybe it closes #3618.

Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the fix. LGTM, see mino comments.

Plus we have a no CI run, see #3600

@@ -85,7 +85,7 @@ protected void handleTargetedExpression(CtTargetedExpression<?, ?> targetedExpre
return;
}

if (target != null && target.isImplicit()) {
if (target.isImplicit()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about keeping target != null to avoid crashing with NPE on target.isImplicit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is redundant with the check just above(here):

if (target == null) {
   ...
   return;
}

If target == null, the condition target.isImplicit() will not be hit, isn't?

@monperrus
Copy link
Collaborator

OK for me. Unwip? Ready for merge?

@danglotb danglotb changed the title WIP: Fix: print import static method Fix: print import static method Dec 9, 2020
@monperrus monperrus changed the title Fix: print import static method Fix: fix pretty-printing of imports for static methods Dec 9, 2020
@monperrus monperrus merged commit 77b8869 into INRIA:master Dec 9, 2020
@monperrus
Copy link
Collaborator

Thanks a lot @danglotb

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

Successfully merging this pull request may close these issues.

2 participants