Skip to content

Commit

Permalink
Only create capture fields when the captured variable is used outside…
Browse files Browse the repository at this point in the history 
… of the initializer.

	Change on 2016/10/13 by kstanger <[email protected]>

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=136033066
  • Loading branch information
@kstanger @tomball
kstanger authored and tomball committed Oct 13, 2016
1 parent 6f3d811 commit d326602
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 66 deletions.
2 changes: 1 addition & 1 deletion cycle_finder/src/main/java/com/google/devtools/cyclefinder/ReferenceGraph.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ private void addAnonymousClassCaptureEdges() {
for (ITypeBinding type : allTypes.values()) {
if (type.isAnonymous()) {
for (VariableElement capturedVarElement :
captureInfo.getInnerFields(
captureInfo.getCaptureFields(
BindingConverter.getTypeElement(type.getTypeDeclaration()))) {
IVariableBinding capturedVarBinding = (IVariableBinding) BindingConverter.unwrapElement(
capturedVarElement);
Expand Down
32 changes: 11 additions & 21 deletions translator/src/main/java/com/google/devtools/j2objc/translate/InnerClassExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.google.devtools.j2objc.ast.UnitTreeVisitor;
import com.google.devtools.j2objc.jdt.BindingConverter;
import com.google.devtools.j2objc.types.GeneratedMethodBinding;
import com.google.devtools.j2objc.types.GeneratedVariableBinding;
import com.google.devtools.j2objc.util.BindingUtil;
import com.google.devtools.j2objc.util.CaptureInfo;
import com.google.devtools.j2objc.util.ErrorUtil;
Expand All @@ -50,7 +49,6 @@
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.VariableElement;
import org.eclipse.jdt.core.dom.ITypeBinding;
import org.eclipse.jdt.core.dom.IVariableBinding;
import org.eclipse.jdt.core.dom.Modifier;

/**
Expand Down Expand Up @@ -145,8 +143,7 @@ private void addOuterFields(AbstractTypeDeclaration node) {
members.add(0, new FieldDeclaration(outerFieldElement, null));
}

List<VariableElement> innerFields = captureInfo.getInnerFields(clazz);
for (VariableElement field : innerFields) {
for (VariableElement field : captureInfo.getCaptureFields(clazz)) {
node.addBodyDeclaration(new FieldDeclaration(field, null));
}
}
Expand Down Expand Up @@ -182,18 +179,9 @@ protected void addOuterParameters(
captureDecls.add(new SingleVariableDeclaration(superOuterParam));
captureTypes.add(BindingConverter.unwrapTypeMirrorIntoTypeBinding(superOuterParam.asType()));
}
List<VariableElement> innerFields = captureInfo.getInnerFields(typeE);
List<IVariableBinding> captureParams = Lists.newArrayListWithCapacity(innerFields.size());
int captureCount = 0;
for (VariableElement innerField : innerFields) {
ITypeBinding innerFieldType =
BindingConverter.unwrapTypeMirrorIntoTypeBinding(innerField.asType());
GeneratedVariableBinding paramBinding = new GeneratedVariableBinding(
"capture$" + captureCount++, Modifier.FINAL, innerFieldType, false, true, type,
constructorBinding);
captureDecls.add(new SingleVariableDeclaration(paramBinding));
captureTypes.add(innerFieldType);
captureParams.add(paramBinding);
for (VariableElement captureParam : captureInfo.getCaptureParams(typeE)) {
captureDecls.add(new SingleVariableDeclaration(captureParam));
captureTypes.add(BindingConverter.unwrapTypeMirrorIntoTypeBinding(captureParam.asType()));
}

ConstructorInvocation thisCall = null;
Expand All @@ -220,9 +208,9 @@ protected void addOuterParameters(
args.add(new SimpleName(outerParam));
params.add(BindingConverter.unwrapTypeMirrorIntoTypeBinding(outerParam.asType()));
}
for (IVariableBinding captureParam : captureParams) {
for (VariableElement captureParam : captureInfo.getCaptureParams(typeE)) {
args.add(new SimpleName(captureParam));
params.add(captureParam.getType());
params.add(BindingConverter.unwrapTypeMirrorIntoTypeBinding(captureParam.asType()));
}
} else {
ITypeBinding superType = type.getSuperclass().getTypeDeclaration();
Expand All @@ -238,9 +226,11 @@ protected void addOuterParameters(
statements.add(idx++, new ExpressionStatement(
new Assignment(new SimpleName(outerField), new SimpleName(outerParam))));
}
for (int i = 0; i < innerFields.size(); i++) {
statements.add(idx++, new ExpressionStatement(new Assignment(
new SimpleName(innerFields.get(i)), new SimpleName(captureParams.get(i)))));
for (CaptureInfo.LocalCapture capture : captureInfo.getLocalCaptures(typeE)) {
if (capture.hasField()) {
statements.add(idx++, new ExpressionStatement(new Assignment(
new SimpleName(capture.getField()), new SimpleName(capture.getParam()))));
}
}
}
assert constructor.getParameters().size()
Expand Down
7 changes: 1 addition & 6 deletions translator/src/main/java/com/google/devtools/j2objc/translate/LambdaRewriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ private void removeCastExpression() {
}

private void replaceNode() {
if (isCapturing()) {
if (captureInfo.isCapturing(lambdaType)) {
node.replaceWith(creation);
} else {
// For non-capturing lambdas, create a static final instance.
Expand All @@ -182,11 +182,6 @@ private Block asImplementationBlock(Expression expr) {
return block;
}

private boolean isCapturing() {
return captureInfo.getOuterField(lambdaType) != null
|| !captureInfo.getInnerFields(lambdaType).isEmpty();
}

private void rewriteLambdaExpression(LambdaExpression node) {
for (VariableDeclaration decl : node.getParameters()) {
implDecl.addParameter(new SingleVariableDeclaration(decl.getVariableElement()));
Expand Down
9 changes: 7 additions & 2 deletions translator/src/main/java/com/google/devtools/j2objc/translate/OuterReferenceResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ private VariableElement getOrCreateOuterVar(Scope scope) {
: captureInfo.getOrCreateOuterField(scope.type);
}

private VariableElement getOrCreateCaptureVar(VariableElement var, Scope scope) {
return scope.isInitializing() ? captureInfo.getOrCreateCaptureParam(var, scope.type)
: captureInfo.getOrCreateCaptureField(var, scope.type);
}

private Name getOuterPath(TypeElement type) {
Name path = null;
for (Scope scope = peekScope(); !type.equals(scope.type); scope = scope.outerClass) {
Expand Down Expand Up @@ -263,7 +268,7 @@ private Expression getPathForLocalVar(VariableElement var) {
lastScope = scope;
}
}
return Name.newName(path, captureInfo.getOrCreateInnerField(var, lastScope.type));
return Name.newName(path, getOrCreateCaptureVar(var, lastScope));
}

private void pushType(TypeElement type) {
Expand All @@ -288,7 +293,7 @@ private void addSuperOuterPath(CommonTypeDeclaration node) {
}

private void addCaptureArgs(TypeElement type, List<Expression> args) {
for (VariableElement var : captureInfo.getCapturedLocals(type)) {
for (VariableElement var : captureInfo.getCapturedVars(type)) {
Expression path = getPathForLocalVar(var);
if (path == null) {
path = new SimpleName(var);
Expand Down
80 changes: 57 additions & 23 deletions translator/src/main/java/com/google/devtools/j2objc/util/CaptureInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.devtools.j2objc.types.GeneratedVariableElement;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -39,14 +39,30 @@ public class CaptureInfo {
private final Map<TypeElement, VariableElement> superOuterParams = new HashMap<>();
private final ListMultimap<TypeElement, LocalCapture> localCaptures = ArrayListMultimap.create();

private static class LocalCapture {
/**
* Information about a captured local variable.
*/
public static class LocalCapture {

private final VariableElement var;
private final VariableElement field;
private final VariableElement param;
private VariableElement field;

private LocalCapture(VariableElement var, VariableElement field) {
private LocalCapture(VariableElement var, VariableElement param) {
this.var = var;
this.field = field;
this.param = param;
}

public VariableElement getParam() {
return param;
}

public boolean hasField() {
return field != null;
}

public VariableElement getField() {
return field;
}
}

Expand Down Expand Up @@ -78,17 +94,26 @@ public VariableElement getOuterField(TypeElement type) {
return outerFields.get(type);
}

public Iterable<VariableElement> getCapturedLocals(TypeElement type) {
public List<LocalCapture> getLocalCaptures(TypeElement type) {
return Collections.unmodifiableList(localCaptures.get(type));
}

public Iterable<VariableElement> getCapturedVars(TypeElement type) {
return Iterables.transform(localCaptures.get(type), capture -> capture.var);
}

public List<VariableElement> getInnerFields(TypeElement type) {
List<LocalCapture> capturesForType = localCaptures.get(type);
List<VariableElement> innerFields = new ArrayList<>(capturesForType.size());
for (LocalCapture capture : capturesForType) {
innerFields.add(capture.field);
}
return innerFields;
public Iterable<VariableElement> getCaptureParams(TypeElement type) {
return Iterables.transform(localCaptures.get(type), capture -> capture.param);
}

public Iterable<VariableElement> getCaptureFields(TypeElement type) {
return Iterables.transform(Iterables.filter(
localCaptures.get(type), LocalCapture::hasField), capture -> capture.field);
}

public boolean isCapturing(TypeElement type) {
return outerFields.containsKey(type)
|| !Iterables.isEmpty(Iterables.filter(localCaptures.get(type), LocalCapture::hasField));
}

private static boolean automaticOuterParam(TypeElement type) {
Expand Down Expand Up @@ -147,23 +172,32 @@ public VariableElement getOrCreateOuterField(TypeElement type) {
return outerField;
}

public VariableElement getOrCreateInnerField(VariableElement var, TypeElement declaringType) {
private LocalCapture getOrCreateLocalCapture(VariableElement var, TypeElement declaringType) {
List<LocalCapture> capturesForType = localCaptures.get(declaringType);
VariableElement innerField = null;
for (LocalCapture capture : capturesForType) {
if (var.equals(capture.var)) {
innerField = capture.field;
break;
for (LocalCapture localCapture : capturesForType) {
if (var.equals(localCapture.var)) {
return localCapture;
}
}
if (innerField == null) {
innerField = new GeneratedVariableElement(
LocalCapture newCapture = new LocalCapture(var, new GeneratedVariableElement(
"capture$" + capturesForType.size(), var.asType(), ElementKind.PARAMETER, declaringType));
capturesForType.add(newCapture);
return newCapture;
}

public VariableElement getOrCreateCaptureParam(VariableElement var, TypeElement declaringType) {
return getOrCreateLocalCapture(var, declaringType).param;
}

public VariableElement getOrCreateCaptureField(VariableElement var, TypeElement declaringType) {
LocalCapture capture = getOrCreateLocalCapture(var, declaringType);
if (capture.field == null) {
capture.field = new GeneratedVariableElement(
getCaptureFieldName(var, declaringType), var.asType(), ElementKind.FIELD, declaringType)
.addModifiers(Modifier.PRIVATE, Modifier.FINAL)
.addAnnotationMirrors(var.getAnnotationMirrors());
localCaptures.put(declaringType, new LocalCapture(var, innerField));
}
return innerField;
return capture.field;
}

public VariableElement createSuperOuterParam(TypeElement type, TypeMirror superOuterType) {
Expand Down
8 changes: 6 additions & 2 deletions ...lator/src/test/java/com/google/devtools/j2objc/translate/AnonymousClassConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -523,8 +523,12 @@ public void testAnonymousClassWithinLambdaWithSuperOuterParam() throws IOExcepti
public void testSuperclassHasCapturedVariables() throws IOException {
String translation = translateSourceFile(
"class Test { static Object test(int i) { class Local { int foo() { return i; } } "
+ "return new Local() { }; } }", "Test", "Test.m");
+ "return new Local() { int bar() { return i; } }; } }", "Test", "Test.m");
// Test that the anonymous class captures i and passes it to Local's constructor.
assertTranslation(translation, "Test_1Local_initWithInt_(self, self->val1$i_);");
assertTranslatedLines(translation,
"void Test_$1_initWithInt_(Test_$1 *self, jint capture$0) {",
" self->val1$i_ = capture$0;",
" Test_1Local_initWithInt_(self, capture$0);",
"}");
}
}
5 changes: 2 additions & 3 deletions translator/src/test/java/com/google/devtools/j2objc/translate/InnerClassExtractorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,11 +597,10 @@ public void testNoOuterWhenInStaticMethod() throws IOException {
+ " public boolean hasMoreElements() { return it.hasNext(); }"
+ " public Object nextElement() { return it.next(); }}; }}";
String translation = translateSourceFile(source, "A", "A.m");
assertFalse(translation.contains("this$0_"));
assertNotInTranslation(translation, "this$0_");
assertTranslation(translation,
"- (instancetype)initWithJavaUtilCollection:(id<JavaUtilCollection>)capture$0;");
assertTranslation(translation, "id<JavaUtilCollection> val$c_;");
assertFalse(translation.contains("this$0_"));
assertTranslation(translation, "[((id<JavaUtilCollection>) nil_chk(capture$0)) iterator]");
assertTranslation(translation,
"return create_A_$1_initWithJavaUtilCollection_(c);");
assertTranslation(translation,
Expand Down
4 changes: 1 addition & 3 deletions translator/src/test/java/com/google/devtools/j2objc/translate/OuterReferenceFixerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ public void testLocalClassExtendsLocalClassCapturesVariables() throws IOExceptio
// Local class B must also capture the locals and pass them to A's constructor.
assertTranslatedLines(translation,
"void Test_1B_initWithInt_withInt_(Test_1B *self, jint capture$0, jint capture$1) {",
" self->val1$i_ = capture$0;",
" self->val1$j_ = capture$1;",
" Test_1A_initWithInt_withInt_(self, self->val1$i_, self->val1$j_);",
" Test_1A_initWithInt_withInt_(self, capture$0, capture$1);",
"}");
}
}
14 changes: 9 additions & 5 deletions ...slator/src/test/java/com/google/devtools/j2objc/translate/OuterReferenceResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.devtools.j2objc.GenerationTest;
import com.google.devtools.j2objc.ast.AnonymousClassDeclaration;
import com.google.devtools.j2objc.ast.ClassInstanceCreation;
Expand Down Expand Up @@ -109,9 +110,10 @@ public void testCapturedLocalVariable() {
AnonymousClassDeclaration runnableNode =
(AnonymousClassDeclaration) nodesByType.get(Kind.ANONYMOUS_CLASS_DECLARATION).get(0);
assertFalse(captureInfo.needsOuterReference(runnableNode.getTypeElement()));
List<VariableElement> innerFields = captureInfo.getInnerFields(runnableNode.getTypeElement());
List<VariableElement> innerFields = Lists.newArrayList(
captureInfo.getCaptureFields(runnableNode.getTypeElement()));
assertEquals(1, innerFields.size());
assertEquals("val$i", innerFields.get(0).getSimpleName().toString());
assertEquals("val$i", ElementUtil.getName(innerFields.get(0)));
ClassInstanceCreation creationNode =
(ClassInstanceCreation) nodesByType.get(Kind.CLASS_INSTANCE_CREATION).get(0);
List<Expression> captureArgs = creationNode.getCaptureArgs();
Expand All @@ -138,7 +140,8 @@ public void testCapturedWeakLocalVariable() {

AnonymousClassDeclaration runnableNode =
(AnonymousClassDeclaration) nodesByType.get(Kind.ANONYMOUS_CLASS_DECLARATION).get(0);
List<VariableElement> innerFields = captureInfo.getInnerFields(runnableNode.getTypeElement());
List<VariableElement> innerFields = Lists.newArrayList(
captureInfo.getCaptureFields(runnableNode.getTypeElement()));
assertEquals(1, innerFields.size());
assertTrue(ElementUtil.isWeakReference(innerFields.get(0)));
}
Expand All @@ -160,9 +163,10 @@ public void testAnonymousClassCreatesLocalClassWithCaptures() {

AnonymousClassDeclaration runnableNode =
(AnonymousClassDeclaration) nodesByType.get(Kind.ANONYMOUS_CLASS_DECLARATION).get(0);
List<VariableElement> innerFields = captureInfo.getInnerFields(runnableNode.getTypeElement());
List<VariableElement> innerFields = Lists.newArrayList(
captureInfo.getCaptureFields(runnableNode.getTypeElement()));
assertEquals(1, innerFields.size());
assertEquals("val$o", innerFields.get(0).getSimpleName().toString());
assertEquals("val$o", ElementUtil.getName(innerFields.get(0)));
ClassInstanceCreation creationNode =
(ClassInstanceCreation) nodesByType.get(Kind.CLASS_INSTANCE_CREATION).get(1);
List<Expression> captureArgs = creationNode.getCaptureArgs();
Expand Down

0 comments on commit d326602

Please sign in to comment.