aaron.ballman added inline comments.
================ Comment at: clang/docs/LanguageExtensions.rst:480 = yes yes yes yes -:? yes -- -- -- +:?[*]_ yes -- yes -- sizeof yes yes yes yes ---------------- Do these columns line up in the actual source file? They don't appear to line up in the Phab viewer, but I don't know if that's an artifact of Phab or not. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6882 +def err_conditional_vector_operand_type + : Error<"%select{enumeral|extended vector}0 type %1 is not allowed in a " + "vector conditional">; ---------------- We don't actually use enumeral in diagnostics anywhere -- I think that should be enumeration instead. ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4301 + llvm::Type *condType = ConvertType(condExpr->getType()); + llvm::VectorType *vecTy = cast<llvm::VectorType>(condType); + llvm::Value *zeroVec = llvm::Constant::getNullValue(vecTy); ---------------- `auto *` Also, should these three variables have identifiers starting with an uppercase like `CondV` et al? ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5757 + const QualType EltTy = + cast<VectorType>(CondTy.getCanonicalType().getTypePtr()) + ->getElementType(); ---------------- Do you actually need the `getTypePtr()` bit, or will the `cast<>` suffice to trigger the cast through `Type*`? ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5761-5762 + // isIntegralType doesn't exactly match the the needs here, but would function + // well enough. The boolean check is redundant since a vector of type bool + // isn't allowed, and the enumeral type check is redundant because + // isIntegralType isn't true in 'C++' mode. Additionally, enumeral types ---------------- If these checks are redundant, would you prefer to assert them? ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5779 + QualType CondType = Cond.get()->getType(); + const VectorType *CondVT = CondType->getAs<VectorType>(); + QualType CondElementTy = CondVT->getElementType(); ---------------- `const auto *` (same elsewhere in the function where the type is spelled out in the initialization) ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5793 + Diag(QuestionLoc, diag::err_conditional_vector_operand_type) + << /*isExtVector*/ true << RHSType; + return {}; ---------------- Shouldn't this pass in the `LHSType` instead because that's what's being tested? ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5799 + Diag(QuestionLoc, diag::err_conditional_vector_operand_type) + << /*isExtVector*/ true << LHSType; + return {}; ---------------- And this one do the `RHSType`? ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:5805 + // If both are vector types, they must be the same type. + if (!Context.hasSameType(LHSType, RHSType)) { + Diag(QuestionLoc, diag::err_conditional_vector_mismatched_vectors) ---------------- Do you need to canonicalize these types before comparing them? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71463/new/ https://reviews.llvm.org/D71463 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits