joechrisellis marked an inline comment as done.
joechrisellis added inline comments.
================
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 =
----------------
fpetrogalli wrote:
> 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.
Simplified the code as per your suggestion, but the lambda here here serves a
purpose: it makes sure that `areLaxCompatibleSveTypes` has the same behaviour
irrespective of the ordering of the parameters. So we do actually need the if
statements inside the lambda.
================
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())
{
----------------
fpetrogalli wrote:
> This code path seems untested.
Thinking about it, this could do with being broken out into its own patch. Will
do this.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1650-1652
+ ICK = ICK_SVE_Vector_Conversion;
+ return true;
+ }
----------------
fpetrogalli wrote:
> tabs!
Not sure where these came from, since I ran `clang-format` over the patch.
Think they should be gone now...
================
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;
----------------
fpetrogalli wrote:
> 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.
> ```
Will break this out into another patch as mentioned above.
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
