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

[Tanya Lattner]

On Feb 12, 2013, at 11:15 AM, Richard Smith <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> 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] On Behalf Of Benyei, Guy
> Sent: Thursday, February 07, 2013 23:22
> To: 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
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> 
> _______________________________________________
> cfe-commits mailing list
> 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/20130212/7da97cd2/attachment.html>