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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
