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