hokein commandeered this revision. hokein added a reviewer: ilya-biryukov. hokein added a comment.
This patch contains too many changes, most of them are just NFC, it likely takes a long time to do a full review. I actually did an review for the original patch. I have highlighted places (see me comments) that I'm uncertain and need another look, other places are NFC. ================ Comment at: clang/include/clang/AST/Expr.h:4081 - : Expr(ConvertVectorExprClass, DstType, VK, OK, - DstType->isDependentType(), - DstType->isDependentType() || SrcExpr->isValueDependent(), ---------------- ! behavior change change, now the `ConvertVectorExpr::TypeDependent = DstType->isDependentType() | SrcExpr->isDependentType()`. ================ Comment at: clang/include/clang/AST/Expr.h:4139 - bool TypeDependent, bool ValueDependent) - : Expr(ChooseExprClass, t, VK, OK, TypeDependent, ValueDependent, - (cond->isInstantiationDependent() || ---------------- ! this needs careful review, the ChooseExpr bits are from different sources: - constructor here - [`clang/lib/Sema/SemaPseudoObject.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaPseudoObject.cpp#L170) - [`clang/lib/AST/ASTImporter.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTImporter.cpp#L6347) - [`clang/lib/Sema/SemaExpr.cpp`](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaExpr.cpp#L14207) ================ Comment at: clang/include/clang/AST/Expr.h:4250 SourceLocation RPLoc, QualType t, bool IsMS) - : Expr(VAArgExprClass, t, VK_RValue, OK_Ordinary, t->isDependentType(), - false, (TInfo->getType()->isInstantiationDependentType() || ---------------- ! the implementation in the original patch doesn't seem to correct to me, I rewrote it, need another review.. ================ Comment at: clang/include/clang/AST/Expr.h:5031 - CommonInit->isValueDependent() || ElementInit->isValueDependent(), - T->isInstantiationDependentType(), - CommonInit->containsUnexpandedParameterPack() || ---------------- ! behavior change here, now `ArrayInitLoopExpr::Instantiation = T->isInstantiationDependentType() | CommonInit->isInstantiationDependentType() | ElementInit->isInstantiationDependentType()`. ================ Comment at: clang/include/clang/AST/Expr.h:5650 - : Expr(AsTypeExprClass, DstType, VK, OK, - DstType->isDependentType(), - DstType->isDependentType() || SrcExpr->isValueDependent(), ---------------- ! behavior change here, now `Expr::TypeDependent = DstType->isDependentType() | SrcExpr->isTypeDependent()`. ================ Comment at: clang/include/clang/AST/Expr.h:1537 CharacterLiteralBits.Kind = kind; + setDependence(ExprDependence::None); } ---------------- this is not needed indeed as the default value is 0, but I'd keep it here to make it more explicit. ================ Comment at: clang/include/clang/AST/ExprCXX.h:2742 - : Expr(ArrayTypeTraitExprClass, ty, VK_RValue, OK_Ordinary, - false, queried->getType()->isDependentType(), - (queried->getType()->isInstantiationDependentType() || ---------------- ! behavior change here, now we the `ValueDependent`, `UnexpandedPack` takes `dimension` into account as well. ================ Comment at: clang/lib/AST/ExprCXX.cpp:444 - SC, Context.OverloadTy, VK_LValue, OK_Ordinary, KnownDependent, - KnownDependent, - (KnownInstantiationDependent || NameInfo.isInstantiationDependent() || ---------------- ! this change is not trivial, need an review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73638/new/ https://reviews.llvm.org/D73638 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
