fpetrogalli added a comment.
Hi @joechrisellis - thank you for this patch!
I have left a couple of comments.
Francesco
================
Comment at: clang/lib/AST/ASTContext.cpp:8563-8566
+ if (const auto *BT = FirstType->getAs<BuiltinType>()) {
+ if (const auto *VT = SecondType->getAs<VectorType>()) {
+ if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
+ const LangOptions::LaxVectorConversionKind LVCKind =
----------------
May I ask to avoid this triple if statement? Given that `BT` is not used after
being defined, I think the following form would be easier to understand:
```
if (!FirstType->getAs<BuiltinType>())
return false;
const auto *VT = SecondType->getAs<VectorType>();
if (VT && VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
/// ...
return ...
}
return false;
```
May I ask you to give meaningful names to the variables? BT and VT are quite
cryptic to me.
Moreover.. are BT and VT really needed? You are asserting
`FirstType->isSizelessBuiltinType() && SecondType->isVectorType()` ... the
`getAs` calls should not fail, no? given that the lambda is local to this
method, I wouldn't bother making it work for the generic case.
================
Comment at: clang/lib/Sema/SemaCast.cpp:2222
if (srcIsVector || destIsVector) {
+ // Scalable vectors can be cast to and from liberally.
+ if (SrcType->isSizelessBuiltinType() || DestType->isSizelessBuiltinType())
{
----------------
This code path seems untested.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1650-1652
+ ICK = ICK_SVE_Vector_Conversion;
+ return true;
+ }
----------------
tabs!
================
Comment at: clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp:19
+
+ // This explicit cast is always allowed, irrespective of the value of
-flax-vector-conversions.
+ ff32 = (fixed_float32_t)sf64;
----------------
Why this one in particular? To me the comment would make more sense if saying
```
// An explicit cast is always allowed, irrespective of the value of
-flax-vector-conversions.
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91067/new/
https://reviews.llvm.org/D91067
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
