[PATCH] D49045: PR15730/PR16986 Allow dependently typed vector_size types.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:1327-1329
+  QualType getDependentVectorType(QualType VectorType, Expr *SizeExpr,
+                                       SourceLocation AttrLoc,
+                                       VectorType::VectorKind VecKind) const;
----------------
The formatting looks off here. Did clang-format do this?


================
Comment at: include/clang/AST/Type.h:3083
 
+/// Represents a vector type where either the type or sizeis dependent.
+////
----------------
sizeis -> size is


================
Comment at: include/clang/AST/Type.h:3105
+public:
+  Expr *getSizeExpr() const { return SizeExpr; }
+  QualType getElementType() const { return ElementType; }
----------------
Can this return `const Expr *` and have a non-const overload to return the non-const `Expr *`?


================
Comment at: lib/AST/ASTContext.cpp:3316
 }
+QualType
+ASTContext::getDependentVectorType(QualType VecType, Expr *SizeExpr,
----------------
Insert a newline above, and reformat.


================
Comment at: lib/AST/ItaniumMangle.cpp:3007-3008
+void CXXNameMangler::mangleNeonVectorType(const DependentVectorType *T) {
+  llvm_unreachable(
+      "Mangling for Dependent Sized Neon Vector not yet implemented");
+}
----------------
This seems reachable.


================
Comment at: lib/AST/ItaniumMangle.cpp:3080
+    const DependentVectorType *T) {
+  llvm_unreachable(
+      "Mangling for Dependent Sized AArch64 Neon Vector not yet implemented");
----------------
As does this.


================
Comment at: lib/AST/Type.cpp:192
+
+void DependentVectorType::Profile(llvm::FoldingSetNodeID &ID,
+                                       const ASTContext &Context,
----------------
Formatting? Also, does this need to take the `VecKind`?


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:1841
+    case Type::DependentVector: {
+      const DependentVectorType *VectorParam =
+          cast<DependentVectorType>(Param);
----------------
Can use `const auto *` here.


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:1844
+
+      if (const VectorType *VectorArg = dyn_cast<VectorType>(Arg)) {
+        // Perform deduction on the element types.
----------------
Here too.


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:1864
+        return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, ArgSize,
+                                             S.Context.IntTy, true, Info,
+                                             Deduced);
----------------
Can the size ever be negative? Perhaps an unsigned type would be better than a signed type?


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:1868
+
+      if (const DependentVectorType *VectorArg =
+              dyn_cast<DependentVectorType>(Arg)) {
----------------
`const auto *`


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:5281
+  case Type::DependentVector: {
+    const DependentVectorType *VecType = cast<DependentVectorType>(T);
+    MarkUsedTemplateParameters(Ctx, VecType->getElementType(), OnlyDeduced,
----------------
`const auto *`


https://reviews.llvm.org/D49045