rjmccall added a comment.

Sorry for the slow review; I'm getting crushed with review requests.



================
Comment at: clang/lib/AST/Expr.cpp:3859
+
+  auto *SubscriptE = dyn_cast<ArraySubscriptExpr>(this);
+  return SubscriptE
----------------
You need to `IgnoreParens()` here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7768
+  if (E->getBase()->getMatrixFromIndexExpr(Info.getLangOpts().MatrixTypes))
+    return false;
+
----------------
This can also occur in the other evaluators due to indexing into an r-value.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5326
+    VK = VK_LValue;
+    OK = OK_VectorComponent;
   } else if (RHSTy->isArrayType()) {
----------------
It should have the value kind of its base.  `get_matrix_rvalue()[0][0]` is 
still an r-value, so you shouldn't allow it to be assigned to.  Needs tests.

I'm fine with re-using `OK_VectorComponent` here, but you should (1) rename it 
to `OK_VectorOrMatrixComponent` and (2) take the opportunity to audit the 
places that use it to make sure they do something sensible.  I expect you'll 
need to at least update some diagnostics about e.g. disallowing taking the 
address of the expression.

I think you should build a new expression node instead of reusing 
`ArraySubscriptExpr`, since basically none of the code that looks for an 
`ArraySubscriptExpr` will do the right thing for your operation.  That will 
also allow you to avoid the "is this actually enabled" check, since you'll only 
see this node when your feature is enabled.  If you're feeling generous, you 
could move vector subscripting to this new node as well and leave 
`ArraySubscriptExpr` exclusively for the standard (or dependent) cases.


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

Reply via email to