On Feb 12, 2013, at 11:15 AM, Richard Smith <[email protected]> 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 <[email protected]> wrote:
> Hi All,
>
> Any comments on this patch?
>
>
>
> Thanks
>
> Guy
>
> <image001.png>
>
>
>
> From: [email protected]
> [mailto:[email protected]] On Behalf Of Benyei, Guy
> Sent: Thursday, February 07, 2013 23:22
> To: [email protected]
> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
