[PATCH] D72281: [Matrix] Add matrix type to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2112
+          llvm::APSInt ArgRows(S.Context.getTypeSize(S.Context.IntTy),
+                               ConstantMatrixArg->getNumRows());
+          Result = DeduceNonTypeTemplateArgument(
----------------
rjmccall wrote:
> fhahn wrote:
> > rjmccall wrote:
> > > Oh, and this needs to use `size_t`; not just the value but the type of a non-type template parameter can be deduced, so you should use an appropriate type.  Test case for this deduction is `template <auto R> void foo(matrix<int, R, 10>);` or something like that, with a check that `decltype(R)` is `size_t` (which you can do in any number of ways).
> > Could we be even more permissive? If we fix it to size_t, template arguments with integer types like int or unsigned would be rejected.  Could we relax that to NTTP type, to allow different integer types that are implicitly convertible? We might need an extra check that the known number of rows/columns does not exceed the bit width of NTTP's type.
> Unfortunately, template argument deduction requires the template parameter type to match the type of the argument value exactly, so you get exactly one type.  Given that language rule, the natural type to use is `size_t`, which is the same type that constant array bounds are considered to have.   If C++ wants to weaken this language rule, they should do so uniformly; it does not make sense for us to use a weaker rule just for this specific extension.
Thanks for the explanation! I've updated the code to just allow size_t.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72281/new/

https://reviews.llvm.org/D72281