c-rhodes added a comment.

@DavidTruby thanks for updating, just one last comment otherwise LGTM



================
Comment at: clang/lib/Sema/SemaChecking.cpp:13591-13595
+    if (!Target->isVLSTBuiltinType() && !isa<VectorType>(OriginalTarget)) {
+      if (S.SourceMgr.isInSystemMacro(CC))
+        return;
+      return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar);
+    }
----------------
can a test be added for this?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13582
+    // Need the original target type for vector type checks
+    const Type *OriginalTarget = S.Context.getCanonicalType(T).getTypePtr();
+    // Handle conversion from scalable to fixed when msve-vector-bits is
----------------
DavidTruby wrote:
> c-rhodes wrote:
> > this is the same as `Target` defined on line 13473
> The control flow in this function is a little odd, but it isn't necessarily 
> the same target; we could have entered one or more of the if statements above 
> that modify the target, and we need to inspect the original target here.
> The control flow in this function is a little odd, but it isn't necessarily 
> the same target; we could have entered one or more of the if statements above 
> that modify the target, and we need to inspect the original target here.

Ah ofc, I missed that, thanks for clarifying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126380

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to