[PATCH] D71463: Implement VectorType conditional operator GNU extension.

[Aaron Ballman via Phabricator via cfe-commits]

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