[PATCH] D30937: [OpenCL] Added diagnostic for checking length of vector

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: lib/Sema/SemaExprMember.cpp:287
 
+// OpenCL spec (Section 6.1.7 Vector Components):
+// The component group notation can occur on the left hand side of an expression. To form an lvalue,
----------------
The format is normally: OpenCL v1.1, s6.1.7


================
Comment at: lib/Sema/SemaExprMember.cpp:288
+// OpenCL spec (Section 6.1.7 Vector Components):
+// The component group notation can occur on the left hand side of an expression. To form an lvalue,
+// swizzling must be applied to an l-value of vector type, contain no duplicate components,
----------------
Does this text have much relation to the function? I don't feel that copying it from the spec is helping much in understanding of this function. It's probably just Ok to have the reference to the spec section and concisely summarize that the component swizzle length must be in accordance with the acceptable vector sizes.

Also would it be better to rename this to something like:

  IsValidOpenCLComponentSwizzleLength


================
Comment at: lib/Sema/SemaExprMember.cpp:389
 
+  if (S.getLangOpts().OpenCL && !HalvingSwizzle) {
+    compStr = CompName->getNameStart();
----------------
I think the logic in this function is all for OpenCL, but we didn't check it . Anyways, let's leave it for now.


================
Comment at: lib/Sema/SemaExprMember.cpp:395
+
+    unsigned swizzleLength = StringRef(compStr).size();
+    if (IsValidSwizzleLength(swizzleLength) == false) {
----------------
Variable name doesn't adhere the coding style:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Also you could use CompName->getLength() instead of constructing a new StringRef object.


================
Comment at: test/SemaOpenCL/vector_swizzle_length.cl:8
+
+    f2.s01234; // expected-error {{vector component access has invalid length 5.  Supported: 1,2,3,4,8,16}}
+}
----------------
Could we add a non-numeric swizzle too?


https://reviews.llvm.org/D30937