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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12139
+    return X;
+  assert(declaresSameEntity(X, Y));
+  for (const Decl *DX : X->redecls()) {
----------------
erichkeane wrote:
> 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.
Actually we want to handle the case they are both null, in which case we return null as well.
This case would trip the assert below, as `declaresSameEntity` considers that not the same entity.

For example, unconstrained AutoType will have null ConceptDecl.

In the D130308 patch, we add another variant, `getCommonDeclChecked, which requires non-null inputs, which can be used in places where non-null is required.

I went ahead and moved the introduction of `getCommonDeclChecked` to this patch, even though there is only one use of it.


================
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)),
----------------
erichkeane wrote:
> Do we REALLY need to tolerate  'null' here as well?  It seems we should do a normal cast and have the cast assert.
Yes, as above.


================
Comment at: clang/lib/AST/ASTContext.cpp:12172
+
+static auto getCommonTypeArray(ASTContext &Ctx, ArrayRef<QualType> Xs,
+                               ArrayRef<QualType> Ys,
----------------
erichkeane wrote:
> 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`.
Yeah that name works as well.


================
Comment at: clang/lib/AST/ASTContext.cpp:12241
+
+template <class T> static auto *getCommonSizeExpr(T *X, T *Y) {
+  assert(X->getSizeExpr() == Y->getSizeExpr());
----------------
erichkeane wrote:
> 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? 
Well the point is to not repeat the assert in all use sites.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283