Rework structs (#66)

A large refactoring to change how structs are handled. Previously we had pretended that structs were stand-alone declarations, captured their types structurally and used aliases to these types throughout the AST. This was OK to get things going but (a) made cloning a pain, and (b) was not fully general. Now we treat structs properly as just part of a variable declaration list. Instead of aliasing types through the AST we have the notion of a named struct type (which says nothing about fields) and a struct definition type (which has field information). A little more work is required to get fields from a named struct type, but the payoff is that cloning is straightforward, and the treatment of structs is a lot more general.
google · Oct 16, 2018 · 9c95a77 · 9c95a77
1 parent cb07c7b
commit 9c95a77
Show file tree

Hide file tree

Showing 53 changed files with 1,350 additions and 1,034 deletions.
diff --git a/ast/src/main/java/com/graphicsfuzz/common/ast/TranslationUnit.java b/ast/src/main/java/com/graphicsfuzz/common/ast/TranslationUnit.java
@@ -17,21 +17,18 @@
 package com.graphicsfuzz.common.ast;
 
 import com.graphicsfuzz.common.ast.decl.Declaration;
-import com.graphicsfuzz.common.ast.decl.StructDeclaration;
 import com.graphicsfuzz.common.ast.decl.VariableDeclInfo;
 import com.graphicsfuzz.common.ast.decl.VariablesDeclaration;
 import com.graphicsfuzz.common.ast.stmt.VersionStatement;
-import com.graphicsfuzz.common.ast.type.QualifiedType;
-import com.graphicsfuzz.common.ast.type.StructType;
+import com.graphicsfuzz.common.ast.type.StructDefinitionType;
+import com.graphicsfuzz.common.ast.type.StructNameType;
 import com.graphicsfuzz.common.ast.type.TypeQualifier;
 import com.graphicsfuzz.common.ast.visitors.IAstVisitor;
-import com.graphicsfuzz.common.ast.visitors.StandardVisitor;
 import com.graphicsfuzz.common.util.ListConcat;
+import com.graphicsfuzz.common.util.StructUtils;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.stream.Collectors;
 
 public class TranslationUnit implements IAstNode {
@@ -91,61 +88,6 @@ public TranslationUnit clone() {
         topLevelDeclarations.stream().map(x -> x.clone()).collect(Collectors.toList()));
   }
 
-  /**
-   * The trouble with cloning is that it duplicates parts of the tree that should really be
-   * shared.  In particular, struct type references should all refer to the declaration.
-   * This method clones the AST and then patches up such issues
-   * @return Cloned and patched up translation unit
-   */
-  public TranslationUnit cloneAndPatchUp() {
-
-    TranslationUnit result = this.clone();
-
-    new StandardVisitor() {
-
-      private Map<String, StructType> mapping;
-
-      @Override
-      public void visitStructDeclaration(StructDeclaration structDeclaration) {
-        super.visitStructDeclaration(structDeclaration);
-        mapping.put(structDeclaration.getType().getName(), structDeclaration.getType());
-      }
-
-      @Override
-      public void visitQualifiedType(QualifiedType qualifiedType) {
-        super.visitQualifiedType(qualifiedType);
-        if (qualifiedType.getTargetType() instanceof StructType) {
-          final String name = ((StructType) qualifiedType.getTargetType()).getName();
-          if (mapping.containsKey(name)) {
-            qualifiedType.setTargetType(mapping.get(name));
-          }
-        }
-      }
-
-      @Override
-      public void visitStructType(StructType structType) {
-        super.visitStructType(structType);
-        for (int i = 0; i < structType.getNumFields(); i++) {
-          if (structType.getFieldType(i) instanceof StructType) {
-            final String name = ((StructType) structType.getFieldType(i)).getName();
-            if (mapping.containsKey(name)) {
-              structType.setFieldType(i, mapping.get(name));
-            }
-          }
-        }
-      }
-
-      public void patchUp(TranslationUnit tu) {
-        mapping = new HashMap<>();
-        visit(tu);
-      }
-
-    }.patchUp(result);
-
-    return result;
-
-  }
-
   public List<VariableDeclInfo> getGlobalVarDeclInfos() {
     return getGlobalVariablesDeclarations()
         .stream()
@@ -168,4 +110,16 @@ public List<VariablesDeclaration> getUniformDecls() {
         .collect(Collectors.toList());
   }
 
+  public List<StructDefinitionType> getStructDefinitions() {
+    return StructUtils.getStructDefinitions(this);
+  }
+
+  public StructDefinitionType getStructDefinition(StructNameType type) {
+    return getStructDefinitions()
+        .stream()
+        .filter(item -> item.hasStructNameType() && item.getStructNameType().equals(type))
+        .findAny()
+        .get();
+  }
+
 }
diff --git a/ast/src/main/java/com/graphicsfuzz/common/ast/decl/StructDeclaration.java b/ast/src/main/java/com/graphicsfuzz/common/ast/decl/StructDeclaration.java
diff --git a/ast/src/main/java/com/graphicsfuzz/common/ast/decl/VariablesDeclaration.java b/ast/src/main/java/com/graphicsfuzz/common/ast/decl/VariablesDeclaration.java
@@ -19,6 +19,7 @@
 import com.graphicsfuzz.common.ast.type.Type;
 import com.graphicsfuzz.common.ast.visitors.IAstVisitor;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -28,15 +29,25 @@ public class VariablesDeclaration extends Declaration {
   private Type baseType;
   private List<VariableDeclInfo> declInfos;
 
-  public VariablesDeclaration(Type baseType, List<VariableDeclInfo> declInfo) {
+  public VariablesDeclaration(Type baseType,
+                              List<VariableDeclInfo> declInfos) {
     assert baseType != null;
     this.baseType = baseType;
-    this.declInfos = declInfo;
+    this.declInfos = new ArrayList<>();
+    this.declInfos.addAll(declInfos);
   }
 
-  public VariablesDeclaration(Type baseType, VariableDeclInfo decl) {
+  public VariablesDeclaration(Type baseType, VariableDeclInfo declInfo) {
+    this(baseType, Arrays.asList(declInfo));
+  }
+
+  /**
+   * Constructs a variables declaration with no variables attached.  Useful for making a lone
+   * struct.
+   * @param baseType The base type for the empty variables declaration.
+   */
+  public VariablesDeclaration(Type baseType) {
     this(baseType, new ArrayList<>());
-    declInfos.add(decl);
   }
 
   public Type getBaseType() {

diff --git a/ast/src/main/java/com/graphicsfuzz/common/ast/type/QualifiedType.java b/ast/src/main/java/com/graphicsfuzz/common/ast/type/QualifiedType.java
@@ -69,7 +69,7 @@ public Type getTargetType() {
     return targetType;
   }
 
-  public void setTargetType(StructType targetType) {
+  public void setTargetType(Type targetType) {
     this.targetType = targetType;
   }
 

diff --git a/...phicsfuzz/common/ast/type/StructType.java → ...common/ast/type/StructDefinitionType.java b/...phicsfuzz/common/ast/type/StructType.java → ...common/ast/type/StructDefinitionType.java
@@ -17,37 +17,42 @@
 package com.graphicsfuzz.common.ast.type;
 
 import com.graphicsfuzz.common.ast.expr.Expr;
-import com.graphicsfuzz.common.ast.expr.TypeConstructorExpr;
 import com.graphicsfuzz.common.ast.visitors.IAstVisitor;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
-public class StructType extends UnqualifiedType {
+public class StructDefinitionType extends UnqualifiedType {
 
-  private String name;
+  private Optional<StructNameType> structNameType;
   private final List<String> fieldNames;
   private final List<Type> fieldTypes;
 
-  /**
-   * Makes a named struct with the given field names and types.
-   *
-   * @param name Name of the struct
-   * @param fieldNames Ordered field names
-   * @param fieldTypes Ordered field types, one per field name
-   */
-  public StructType(String name, List<String> fieldNames, List<Type> fieldTypes) {
-    assert fieldNames.size() == fieldTypes.size();
-    this.name = name;
+  public StructDefinitionType(Optional<StructNameType> structNameType,
+                               List<String> fieldNames,
+                               List<Type> fieldTypes) {
+    this.structNameType = structNameType;
     this.fieldNames = new ArrayList<>();
     this.fieldNames.addAll(fieldNames);
     this.fieldTypes = new ArrayList<>();
     this.fieldTypes.addAll(fieldTypes);
   }
 
-  public String getName() {
-    return name;
+  public StructDefinitionType(StructNameType structNameType,
+                              List<String> fieldNames,
+                              List<Type> fieldTypes) {
+    this(Optional.of(structNameType), fieldNames, fieldTypes);
+  }
+
+  public boolean hasStructNameType() {
+    return structNameType.isPresent();
+  }
+
+  public StructNameType getStructNameType() {
+    assert hasStructNameType();
+    return structNameType.get();
   }
 
   public List<String> getFieldNames() {
@@ -102,60 +107,6 @@ public void insertField(int index, String name, Type type) {
     fieldTypes.add(index, type);
   }
 
-  @Override
-  public void accept(IAstVisitor visitor) {
-    visitor.visitStructType(this);
-  }
-
-  @Override
-  public StructType clone() {
-    List<String> newFieldNames = new ArrayList<>();
-    List<Type> newFieldTypes = new ArrayList<>();
-    newFieldNames.addAll(fieldNames);
-    newFieldTypes
-        .addAll(fieldTypes.stream().map(item -> item.clone()).collect(Collectors.toList()));
-    return new StructType(name, newFieldNames, newFieldTypes);
-  }
-
-  @Override
-  public boolean hasCanonicalConstant() {
-    for (Type t : fieldTypes) {
-      if (!t.hasCanonicalConstant()) {
-        return false;
-      }
-    }
-    return true;
-  }
-
-  @Override
-  public Expr getCanonicalConstant() {
-    return new TypeConstructorExpr(name,
-        fieldTypes.stream().map(item -> item.getCanonicalConstant())
-            .collect(Collectors.toList()));
-  }
-
-  @Override
-  public boolean equals(Object that) {
-    if (this == that) {
-      return true;
-    }
-    if (!(that instanceof StructType)) {
-      return false;
-    }
-    StructType thatStruct = (StructType) that;
-    return name.equals(thatStruct.name)
-        && fieldNames.equals(thatStruct.fieldNames)
-        && fieldTypes.equals(thatStruct.fieldTypes);
-  }
-
-  @Override
-  public int hashCode() {
-    // TODO: revisit if we find performance is an issue and need a better hash code
-    return name.hashCode()
-        ^ fieldNames.hashCode()
-        ^ fieldTypes.hashCode();
-  }
-
   public void removeField(String fieldToRemove) {
     if (!fieldNames.contains(fieldToRemove)) {
       throw new IllegalArgumentException(unknownFieldMessage(fieldToRemove));
@@ -186,11 +137,31 @@ public boolean hasField(String nameToCheck) {
 
   private String unknownFieldMessage(String fieldName) {
     return "Field " + fieldName + " not found in struct"
-        + " type " + name;
+        + (hasStructNameType() ? " " + structNameType : "") + ".";
   }
 
-  public void setName(String name) {
-    assert name != null;
-    this.name = name;
+  @Override
+  public void accept(IAstVisitor visitor) {
+    visitor.visitStructDefinitionType(this);
   }
+
+  @Override
+  public StructDefinitionType clone() {
+    return new StructDefinitionType(structNameType.map(StructNameType::clone),
+        fieldNames,
+        fieldTypes.stream().map(item -> item.clone()).collect(Collectors.toList()));
+  }
+
+  @Override
+  public boolean hasCanonicalConstant() {
+    // TODO: add generation of canonical constants.
+    return false;
+  }
+
+  @Override
+  public Expr getCanonicalConstant() {
+    throw new UnsupportedOperationException("Support for canonical struct constants not yet added"
+        + ".");
+  }
+
 }