[libcxx-commits] [PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via libcxx-commits]

davrec added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12218
+        CTN,
+        NExpX && NExpY ? Optional<unsigned>(std::min(*NExpX, *NExpY)) : None);
+  }
----------------
I'm not clear on how `NExpX` could not equal `NExpY` - could you add a test which demonstrates this case?


================
Comment at: clang/lib/AST/ASTContext.cpp:12804
+      return QualType();
+    SmallVector<TemplateArgument, 8> As;
+    if (getCommonTemplateArguments(Ctx, As, TX->template_arguments(),
----------------
Nit: "As" -> "Args", since that's common elsewhere


================
Comment at: clang/lib/AST/ASTContext.cpp:12872
   }
 
   SplitQualType SX = X.split(), SY = Y.split();
----------------
It would be very helpful to incorporate your description and the description from D111283 as comments in this function.  E.g. something like the following ...


================
Comment at: clang/lib/AST/ASTContext.cpp:12874
   SplitQualType SX = X.split(), SY = Y.split();
-  if (::removeDifferentTopLevelSugar(SX, SY))
-    SX.Ty = ::getCommonType(*this, SX.Ty, SY.Ty).getTypePtr();
-
+  Qualifiers QX, QY;
+  auto Xs = ::unwrapSugar(SX, QX), Ys = ::unwrapSugar(SY, QY);
----------------
```
// Desugar SX and SY, setting the sugar and qualifiers aside into Xs and Ys/QX and QY, 
// until we reach their underlying "canonical nodes".  (Note these are not necessarily 
// canonical types, as their child types may still be sugared.)
```


================
Comment at: clang/lib/AST/ASTContext.cpp:12876
+  auto Xs = ::unwrapSugar(SX, QX), Ys = ::unwrapSugar(SY, QY);
+  if (SX.Ty != SY.Ty) {
+    SX.Ty = ::getCommonNonSugarTypeNode(*this, SX.Ty, SY.Ty).getTypePtr();
----------------
```
// The canonical nodes differ. Build a common canonical node out of the two, 
// including any sugar shared by their child types.
```


================
Comment at: clang/lib/AST/ASTContext.cpp:12878
+    SX.Ty = ::getCommonNonSugarTypeNode(*this, SX.Ty, SY.Ty).getTypePtr();
+  } else {
+    while (!Xs.empty() && !Ys.empty() && Xs.back().Ty == Ys.back().Ty) {
----------------
```
// The canonical nodes were identical: we may have desugared too much.
// Add any common sugar back in.
```


================
Comment at: clang/lib/AST/ASTContext.cpp:12890
+    assert(QX == QY);
+
+  while (!Xs.empty() && !Ys.empty()) {
----------------
```
// Even though the remaining sugar nodes in Xs and Ys differ, some may be of the same
// type class and have common sugar in their child types.  Walk up these nodes, 
// adding in any such sugar.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130308/new/

https://reviews.llvm.org/D130308