wuzish added inline comments.
================ Comment at: clang/lib/Sema/SemaOverload.cpp:3913 + for (auto Type : Types) { + if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector) + return false; ---------------- hubert.reinterpretcast wrote: > wuzish wrote: > > hubert.reinterpretcast wrote: > > > wuzish wrote: > > > > hubert.reinterpretcast wrote: > > > > > Considering that this is a local lambda called in one place, are we > > > > > expecting cases where the canonical type is not something on which we > > > > > can call getVectorKind()? If not, then we do not need this `if`. > > > > Well, that's for ExtVectorType. I encounter some testcase failure > > > > because of that. So I just narrow the condition to only handle > > > > Type::Vector. > > > > > > > > As the following comment said, so I treat it separately. > > > > > > > > /// ExtVectorType - Extended vector type. This type is created using > > > > /// __attribute__((ext_vector_type(n)), where "n" is the number of > > > > elements. > > > > /// Unlike vector_size, ext_vector_type is only allowed on typedef's. > > > > This > > > > /// class enables syntactic extensions, like Vector Components for > > > > accessing > > > > /// points (as .xyzw), colors (as .rgba), and textures (modeled after > > > > OpenGL > > > > /// Shading Language). > > > An ExtVectorType is a VectorType, so what sort of failures are you > > > hitting? > > Yes. But the TypeClass of ExtVectorType is ExtVector. > > > > some test points check the error report for ambiguous call because of too > > many implicit cast choices from ext_vector_type to vector type. Such as > > blow. > > > > > > ``` > > typedef char char16 __attribute__ ((__vector_size__ (16))); > > typedef long long longlong16 __attribute__ ((__vector_size__ (16))); > > typedef char char16_e __attribute__ ((__ext_vector_type__ (16))); > > typedef long long longlong16_e __attribute__ ((__ext_vector_type__ (2))); > > > > > > void f1_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e > > ll16e) { > > int &ir1 = f1(c16); > > float &fr1 = f1(ll16); > > f1(c16e); // expected-error{{call to 'f1' is ambiguous}} > > f1(ll16e); // expected-error{{call to 'f1' is ambiguous}} > > } > > ``` > > > > If we are gonna widen the condition, we can make a follow-up patch. Or we > > need include this condition and do this together in this patch? > The widening that has occurred is in allowing the scope of the change to > encompass cases where AltiVec vectors are not sufficiently involved. The > change in behaviour makes sense, and perhaps the community may want to pursue > it; however, the mandate to make that level of change has not been given. > > I do not believe that requiring that the TypeClass be VectorType is the > correct narrowing of the scope. Instead, we may want to consider requiring > that for each `SCS` in { `SCS1`, `SCS2` }, there is one AltiVec type and one > generic vector type in { `SCS.getFromType()`, `SCS.getToType(2)` }. > The key point is that ExtVector is a kind of typeclass, **and** its vector kind is generic vector type. So you think we should encompass ext_vector cases as a part of the scope of this patch? And modify the above cases' expected behavior (remove the **expected-error**)? https://reviews.llvm.org/D53417 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits