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

Test case and fix for Issue 1027. Add Google Inc. as contributor. #1038

Merged
merged 4 commits into from Feb 14, 2017

Conversation

wmdietlGC
Copy link
Contributor

No description provided.

@wmdietlGC wmdietlGC mentioned this pull request Feb 14, 2017
@@ -1124,6 +1124,7 @@ \section{Credits and changelog\label{credits}}
David Lazar,
David McArthur,
Eric Spishak,
Google Inc. (via @wmdietlGC),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please put \ after . to signal to LaTeX that this period doesn't end a sentence.

TypeMirror enclosingType = ElementUtils.enclosingClass(ele).asType();
FlowExpressionContext parameterContext =
FlowExpressionContext.buildContextForMethodDeclaration(
methodTree, enclosingType, factory.getContext());
standardizeDoNotUseLocals(parameterContext, path, type);
break;
} else {
// If there is no enclosing method, then the parameter is a parameter to a lambda
Copy link
Member

Choose a reason for hiding this comment

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

It's not that there is no enclosing method, but that the nearest enclosing thing was a lambda rather than a method. Am I right about that?
(I realize that you copied the comment from the existing code, but I think it can still be improved.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there might still be a method around the lambda, but the most enclosing tree was a lambda.
I've added short comments to both branches.
I think enclTree will always be non-null; I can't think of a way to get a parameter without either of these two cases being true. We'll see if we ever get a NPE on the if condition.

@mernst
Copy link
Member

mernst commented Feb 14, 2017

Looks great, thanks a lot!
I have just two minor nits, then this can be merged.

@mernst mernst merged commit aff0922 into typetools:master Feb 14, 2017
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.

None yet

2 participants