Address review comments

- Avoid unnecessary memory allocation
- Fix spelling errors
- Some further minor fixes

Author: swiftix

include/swift/AST/Substitution.h

@@ -48,7 +48,7 @@ class Substitution {
 
   Substitution(Type Replacement, ArrayRef<ProtocolConformanceRef> Conformance);
 
-  /// Checks whether the current substituion is canonical.
+  /// Checks whether the current substitution is canonical.
   bool isCanonical() const;
 
   /// Get the canonicalized substitution. If wasCanonical is not nullptr,

include/swift/AST/SubstitutionList.h

@@ -29,9 +29,18 @@ using SubstitutionList = ArrayRef<Substitution>;
 
 void dump(SubstitutionList subs);
 
-/// Create a canonicalized substitution list.
-/// It may return the argument substitution list if it is canonical already.
-SubstitutionList getCanonicalSubstitutionList(SubstitutionList subs);
+/// Create a canonicalized substitution list from subs.
+/// subs is the substitution list to be canonicalized.
+/// canSubs is an out-parameter, which is used to store the results in case
+/// the list of substitutions was not canonical.
+/// The function returns a list of canonicalized substitutions.
+/// If the substitution list subs was canonical already, it will be returned and
+/// canSubs out-parameter will be empty.
+/// If something had to be canonicalized, then the canSubs out-parameter will be
+/// populated and the returned SubstitutionList would refer to canSubs storage.
+SubstitutionList
+getCanonicalSubstitutionList(SubstitutionList subs,
+                             SmallVectorImpl<Substitution> &canSubs);
 } // end namespace swift
 
 #endif

lib/AST/ASTContext.cpp

@@ -4189,7 +4189,8 @@ CanSILBoxType SILBoxType::get(ASTContext &C,
   llvm::FoldingSetNodeID id;
 
   // Canonicalize substitutions.
-  Args = getCanonicalSubstitutionList(Args);
+  SmallVector<Substitution, 4> CanArgs;
+  Args = getCanonicalSubstitutionList(Args, CanArgs);
 
   Profile(id, Layout, Args);
 

lib/AST/ProtocolConformance.cpp

@@ -1062,19 +1062,17 @@ DeclContext::getLocalConformances(
 
 /// Check of all types used by the conformance are canonical.
 bool ProtocolConformance::isCanonical() const {
-  // Normal conformances are considered canonical by
-  // construction.
-  if (getKind() != ProtocolConformanceKind::Normal) {
-    if (!getType()->isCanonical())
-      return false;
-    if (!getInterfaceType()->isCanonical())
-      return false;
-  }
+  // Normal conformances are always canonical by construction.
+  if (getKind() == ProtocolConformanceKind::Normal)
+    return true;
+
+  if (!getType()->isCanonical())
+    return false;
+  if (!getInterfaceType()->isCanonical())
+    return false;
 
   switch (getKind()) {
   case ProtocolConformanceKind::Normal: {
-    // Normal conformances are considered canonical by
-    // construction.
     return true;
   }
   case ProtocolConformanceKind::Inherited: {
@@ -1136,20 +1134,25 @@ Substitution Substitution::getCanonicalSubstitution(bool *wasCanonical) const {
   return *this;
 }
 
-SubstitutionList swift::getCanonicalSubstitutionList(SubstitutionList subs) {
-  SmallVector<Substitution, 4> newCanSubstitutions;
+SubstitutionList
+swift::getCanonicalSubstitutionList(SubstitutionList subs,
+                                    SmallVectorImpl<Substitution> &canSubs) {
   bool subListWasCanonical = true;
   for (auto &sub : subs) {
     bool subWasCanonical = false;
     auto canSub = sub.getCanonicalSubstitution(&subWasCanonical);
     if (!subWasCanonical)
       subListWasCanonical = false;
-    newCanSubstitutions.push_back(canSub);
+    canSubs.push_back(canSub);
   }
-  if (subListWasCanonical)
+
+  if (subListWasCanonical) {
+    canSubs.clear();
     return subs;
-  return newCanSubstitutions[0].getReplacement()->getASTContext().AllocateCopy(
-      newCanSubstitutions);
+  }
+
+  subs = canSubs;
+  return subs;
 }
 
 /// Check of all types used by the conformance are canonical.
@@ -1159,7 +1162,7 @@ ProtocolConformance *ProtocolConformance::getCanonicalConformance() {
 
   switch (getKind()) {
   case ProtocolConformanceKind::Normal: {
-    //assert(isCanonical() && "Normal conformances should always be canonical");
+    // Normal conformances are always canonical by construction.
     return this;
   }
 
@@ -1178,10 +1181,12 @@ ProtocolConformance *ProtocolConformance::getCanonicalConformance() {
     auto spec = cast<SpecializedProtocolConformance>(this);
     auto genericConformance = spec->getGenericConformance();
     auto specSubs = spec->getGenericSubstitutions();
-    auto canSpecSubs = getCanonicalSubstitutionList(specSubs);
+    SmallVector<Substitution, 4> newSpecSubs;
+    auto canSpecSubs = getCanonicalSubstitutionList(specSubs, newSpecSubs);
     return Ctx.getSpecializedConformance(
         getType()->getCanonicalType(),
-        genericConformance->getCanonicalConformance(), canSpecSubs);
+        genericConformance->getCanonicalConformance(),
+        newSpecSubs.empty() ? canSpecSubs : Ctx.AllocateCopy(canSpecSubs));
   }
   }
   llvm_unreachable("bad ProtocolConformanceKind");

lib/SIL/TypeLowering.cpp

@@ -2492,13 +2492,6 @@ TypeConverter::checkFunctionForABIDifferences(SILFunctionType *fnTy1,
   return ABIDifference::Trivial;
 }
 
-void canonicalizeSubstitutions(MutableArrayRef<Substitution> subs) {
-  for (auto &sub : subs) {
-    sub = Substitution(sub.getReplacement()->getCanonicalType(),
-                       sub.getConformances());
-  }
-}
-
 CanSILBoxType
 TypeConverter::getInterfaceBoxTypeForCapture(ValueDecl *captured,
                                              CanType loweredInterfaceType,
@@ -2536,8 +2529,7 @@ TypeConverter::getInterfaceBoxTypeForCapture(ValueDecl *captured,
       return ProtocolConformanceRef(conformedTy->getDecl());
     },
     genericArgs);
-  canonicalizeSubstitutions(genericArgs);
-  
+
   auto boxTy = SILBoxType::get(C, layout, genericArgs);
 #ifndef NDEBUG
   // FIXME: Map the box type out of context when asserting the field so