[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11989
+  else
+    return false;
+
----------------
rjmccall wrote:
> fhahn wrote:
> > rjmccall wrote:
> > > I would suggest checking some preconditions and then just calling `PrepareScalarCast`.
> > > 
> > > You should allow implicit conversions from class types, which somewhat surprisingly I'm not sure we have a convenient method for, but which you can find workable code for in `ConvertForConditional` in SemaExprCXX.cpp.  Test case is `struct DoubleWrapper { operator double(); };`, and you should test using that even when the element type isn't a double.
> > > 
> > > In SemaOverload, you should add builtin candidates for these operators if one operand or the other is a matrix type.  Basically:
> > > 
> > > 1. Collect matrix types in `AddTypesConvertedFrom`
> > > 2. For each matrix type `M` on the LHS, add candidates for `(M, M) -> M` and `(M, E) -> M`, and then analogously on the RHS.  Although you might need to avoid adding redundant candidates if the same type shows up on both sides.
> > Thank you very much John, I hope I managed to update the patch properly according to your comments!
> Does it work to just do the initialization step instead of separately checking for an arithmetic type?  That code is definitely capable of doing arithmetic conversions.
Yes it does. Much simpler, thanks!


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9173
       OpBuilder.addBinaryPlusOrMinusPointerOverloads(Op);
-      OpBuilder.addGenericBinaryArithmeticOverloads();
+      OpBuilder.addGenericBinaryArithmeticOverloads(Op);
     }
----------------
rjmccall wrote:
> Maybe you should just extract your new code into its own method and call it here.  That might fit in a bit cleaner to the design, since unlike the vector case you're not planning on uniformly having element-wise versions of all of the operators.
Sounds good, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76793