[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Thanks for your patience, this was a sizable review that it took a while to be able to make time for.  A handful of non-functional reviews, but the functionality looks fine.



================
Comment at: clang/lib/AST/ASTContext.cpp:12139
+    return X;
+  assert(declaresSameEntity(X, Y));
+  for (const Decl *DX : X->redecls()) {
----------------
As a nit, I'd prefer this assert above the 'if' above, that way it catches Nulls in that case. It seems that the 'contract' for this function is no null values, so we want to catch this ASAP.


================
Comment at: clang/lib/AST/ASTContext.cpp:12154
+T *getCommonDecl(T *X, T *Y) {
+  return cast_or_null<T>(
+      getCommonDecl(const_cast<Decl *>(cast_or_null<Decl>(X)),
----------------
Do we REALLY need to tolerate  'null' here as well?  It seems we should do a normal cast and have the cast assert.


================
Comment at: clang/lib/AST/ASTContext.cpp:12172
+
+static auto getCommonTypeArray(ASTContext &Ctx, ArrayRef<QualType> Xs,
+                               ArrayRef<QualType> Ys,
----------------
Please comment these functions, it isn't clear on read through what you mean by 'Array' here.  You probably mean for this to be something like `getCommonTypes`.


================
Comment at: clang/lib/AST/ASTContext.cpp:12204
+  }
+  llvm_unreachable("");
+}
----------------
Please give me a message here, just anything reasonably descriptive so that when it shows up I have something to grep.


================
Comment at: clang/lib/AST/ASTContext.cpp:12241
+
+template <class T> static auto *getCommonSizeExpr(T *X, T *Y) {
+  assert(X->getSizeExpr() == Y->getSizeExpr());
----------------
I guess I don't see the po int of the next 3 functions?  There is no sugaring or anything, AND they already assume they are the same expression/element/etc? 


================
Comment at: clang/lib/AST/ASTContext.cpp:12323
+  case EST_NoThrow:
+    llvm_unreachable("handled above");
+
----------------
something a little more unique in this (and others!) would be appreciated.


================
Comment at: clang/lib/AST/ASTContext.cpp:12116
+    // If we reach the canonical declaration, then Y is older.
+    if (DX->isCanonicalDecl())
+      return Y;
----------------
davrec wrote:
> I think "canonical" should be replaced with "first" here and `isCanonicalDecl()` with `isFirstDecl()`.  So far as I can tell `getCanonicalDecl()` returns `getFirstDecl()` everywhere for now, but that could conceivably change, and in any case the goal of this code is to find which is older, so "first" would be clearer as well.
For class types, canonical decl is usually the definition if one exists.  So I assume we do mean 'first' here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283