rjmccall added inline comments.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4650
+ (Base->isTypeDependent() || RowIdx->isTypeDependent() ||
+ (ColumnIdx && ColumnIdx->isTypeDependent())))
+ return new (Context) MatrixSubscriptExpr(Base, RowIdx, ColumnIdx,
----------------
fhahn wrote:
> rjmccall wrote:
> > Checking dependence is actually just as cheap as checking for C++, there's
> > no real need to gate. But you need to check for placeholder types in the
> > index operands before doing these type checks. The best test case here is
> > an Objective-C property reference, something like:
> >
> > ```
> > __attribute__((objc_root_class))
> > @interface IntValue
> > @property int value;
> > @end
> >
> > double test(double4x4 m, IntValue *iv) {
> > return m[iv.value][iv.value];
> > }
> > ```
> >
> > Also, I would suggest not doing any checking on incomplete matrix
> > expressions; just notice that it's still incomplete, build the expression,
> > and return. You can do the checking once when you have all the operands
> > together.
> > Checking dependence is actually just as cheap as checking for C++, there's
> > no real need to gate. But you need to check for placeholder types in the
> > index operands before doing these type checks. The best test case here is
> > an Objective-C property reference, something like:
>
> Done, I've added a ActOnMatrixSubscriptExpr and added code to deal with
> placeholder types there.
>
> > Also, I would suggest not doing any checking on incomplete matrix
> > expressions; just notice that it's still incomplete, build the expression,
> > and return. You can do the checking once when you have all the operands
> > together.
>
> Done, I initially thought it might be good to still raise an error if the row
> index was invalid, but given that it's not a valid expression anyways that
> probably is not too helpful anyways. Doing the checks on the complete
> expression simplifies things a bit :)
Placeholders actually need to be handled in the `Build` function — they can
come up in template instantiation, too.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4645
+ // Separating the index expressions by parenthesis is not allowed.
+ if (isa<ParenExpr>(Base) && Base->getType()->isSpecificPlaceholderType(
+ BuiltinType::IncompleteMatrixIdx)) {
----------------
Don't check for `ParenExpr` specifically; there are other expressions that are
handled the same way, like `_Generic`. You need to check for
`!isa<MatrixSubscriptExpr>`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4655
+ // type, in which case the overload resolution for the operator overload
+ // should get the first crack at the overload.
+
----------------
Overload placeholder types are used for things like `&foo` where `foo` is the
name of an overloaded function. The places that resolve only non-overload
placeholder types are doing so in order to leave room for overload resolution
to resolve the overload later, e.g. as part of non-builtin operator handling.
`operator[]` is like `operator()`: non-builtin operators are only considered
when the base has class type. Since you already know that the base type is a
matrix type, you know that you're using your standard rules, and your standard
rules have no way to resolve overloads in the index types — correctly, since
indexes can't be functions or member pointers.
tl;dr: You can (and should) resolve all placeholders here, not just
non-overload placeholders.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76791/new/
https://reviews.llvm.org/D76791
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
