davrec added a comment. > The second paragraph is talking about 'Canonical nodes', not 'Canonical > types'. > > A canonical node is a type node for which 'isSugared' method returns false.
Thanks for the clarification, but note that that term is not in general use so far as I'm aware. But instead of defining it and getting into details it might be clearer, and certainly would have saved me the most time, for the description to simply note that this patch introduces `ASTContext::getCommonSugaredType(QualType X, QualType Y, bool Unqualified = false)`, and puts it to use in type deduction for binary expressions etc., and give an example or two to demonstrate. (More generally on a large patch I prefer a description to give me a few starting points, the primary changes which necessitate all the others.) Re the patch itself: it looks good to me other than a few nits, but this has such a broad and deep span (intricate details of the AST + intricate details of Sema) it is difficult to give it a final thumbs up - really hoping @rsmith might take a final look. But if time runs out it is definitely worth accepting as is and seeing how it goes; the benefits exceed the risks. I will try to take a close look at the other patches on your stack, but the same probably applies. You've done a tremendous amount of work here, very impressive. ================ Comment at: clang/include/clang/AST/ASTContext.h:2807 + FunctionProtoType::ExceptionSpecInfo + getCommonExceptionSpec(FunctionProtoType::ExceptionSpecInfo ESI1, ---------------- A clearer name might be `combineExceptionSpecs`, or the original `mergeExceptionSpecs`, since this is getting the union of their sets of exception specs, whereas getCommon* suggests getting the intersection, e.g. getCommonSugaredType is getting the intersection of two "sets" of type sugar in a sense. Also, please add some brief documentation to the function. ================ Comment at: clang/include/clang/AST/ASTContext.h:2825 + // the common sugared type between them. + void mergeTypeLists(SmallVectorImpl<QualType> &Out, ArrayRef<QualType> X, + ArrayRef<QualType> Y); ---------------- Any reason this is public? Or in the header at all? Seems like it could be a static function in the cpp. ================ Comment at: clang/lib/AST/ASTContext.cpp:12116 + // If we reach the canonical declaration, then Y is older. + if (DX->isCanonicalDecl()) + return Y; ---------------- 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. 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
