[PATCH] OpenCL images and samplers related restriction (6.9.b)

[Benyei, Guy]

Tanya and Richard,
Thanks for the review - attached the updated patch.

Please review

Thank
      Guy
[email_signature_guy_new2]

From: Tanya Lattner [mailto:lattner at apple.com]
Sent: Wednesday, February 13, 2013 00:35
To: Benyei, Guy
Cc: cfe-commits at cs.uiuc.edu; Richard Smith
Subject: Re: [PATCH] OpenCL images and samplers related restriction (6.9.b)


On Feb 12, 2013, at 11:15 AM, Richard Smith <richard at metafoo.co.uk<mailto:richard at metafoo.co.uk>> wrote:


+def err_image_type_usage : Error<
+  "%select{image|sampler}0 type cannot be used to declare "
+  "%select{a variable|a structure or a union field|an array of %select{images|samplers}0|"
+  "a pointer to %select{an image|a sampler}0|the return type of a function}1">;

The diagnostic wording here could be improved:
 * Maybe reverse the word order: "cannot declare a variable of %select{image|sampler}0 type %1", "cannot form an array of image type %0"?
 * Include the actual type in the diagnostic.
 * Don't say 'a structure or a union field', find out which one.

+    // OpenCL v1.2 s6.9.b p2:
+    // An image type cannot be used to declare a variable, a structure or union
+    // field, an array of images, a pointer to an image, or the return type of
+    // a function.

Please indent the standard quotation a little. Also trim out the irrelevant parts with [...] to make it more obvious which part you're checking here.

I also would like to add that you can merge the sections together to something like:
// OpenCL v1.2, s6.9 p2/p4: Images and samples cannot be ...

That would shorten up the comments and still be very descriptive.

Otherwise, I have no additional comments to add as Richard already gave a good review.

-Tanya








+    if (R->isImageType()) {
+      Diag(D.getIdentifierLoc(), diag::err_image_type_usage) << 0 << 0;
+    }

No braces here. Please add a comment explaining what these magic numbers mean.

+  if (LangOpts.OpenCL && (NewFD->getResultType()->isImageType() ||
+    NewFD->getResultType()->isSamplerT())) {

More indent needed here; line this up with the NewFD in the line above.

+    Diag(D.getIdentifierLoc(), diag::err_image_type_usage) <<

Put the << on the next line.

+    ( NewFD->getResultType()->isImageType() ? 0 : 1 ) << 4;

Indent the continuation line. No spaces inside (). Again, please explain what the '4' means.

+    D.setInvalidType();

Is this necessary?

(Likewise, mutatis mutandis, for the other three changes.)

On Tue, Feb 12, 2013 at 1:16 AM, Benyei, Guy <guy.benyei at intel.com<mailto:guy.benyei at intel.com>> wrote:
Hi All,
Any comments on this patch?

Thanks
   Guy
<image001.png>

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu>] On Behalf Of Benyei, Guy
Sent: Thursday, February 07, 2013 23:22
To: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: [PATCH] OpenCL images and samplers related restriction (6.9.b)

Hi all,
Attached the implementation of OpenCL restrictions 6.9.b. It includes image and sampler restrictions.

Please review.

Thanks
<image001.png>


---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130214/c52fd8b6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 24800 bytes
Desc: image001.png
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130214/c52fd8b6/attachment.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opencl_restriction_b2.patch
Type: application/octet-stream
Size: 9552 bytes
Desc: opencl_restriction_b2.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130214/c52fd8b6/attachment.obj>
-------------- next part --------------
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130214/c52fd8b6/attachment-0001.html>