<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Feb 12, 2013, at 11:15 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>+def err_image_type_usage : Error<</div><div>+ "%select{image|sampler}0 type cannot be used to declare "</div><div>+ "%select{a variable|a structure or a union field|an array of %select{images|samplers}0|"</div>
<div>+ "a pointer to %select{an image|a sampler}0|the return type of a function}1">;</div><div><br></div><div>The diagnostic wording here could be improved:</div><div> * Maybe reverse the word order: "cannot declare a variable of %select{image|sampler}0 type %1", "cannot form an array of image type %0"?</div>
<div> * Include the actual type in the diagnostic.</div><div> * Don't say 'a structure or a union field', find out which one.</div><div><br></div><div><div>+ // OpenCL v1.2 s6.9.b p2:</div><div>+ // An image type cannot be used to declare a variable, a structure or union</div>
<div>+ // field, an array of images, a pointer to an image, or the return type of</div><div>+ // a function.</div></div><div><br></div><div>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.</div></blockquote><div><br></div><div>I also would like to add that you can merge the sections together to something like:</div><div>// OpenCL v1.2, s6.9 p2/p4: Images and samples cannot be …</div><div><br></div><div>That would shorten up the comments and still be very descriptive.</div><div><br></div><div>Otherwise, I have no additional comments to add as Richard already gave a good review.</div><div><br></div><div>-Tanya</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><br><blockquote type="cite">
<div><br></div><div><div>+ if (R->isImageType()) {</div><div>+ Diag(D.getIdentifierLoc(), diag::err_image_type_usage) << 0 << 0;</div><div>+ }</div></div><div><br></div><div>No braces here. Please add a comment explaining what these magic numbers mean.</div>
<div><br></div><div><div>+ if (LangOpts.OpenCL && (NewFD->getResultType()->isImageType() ||</div><div>+ NewFD->getResultType()->isSamplerT())) {</div><div><br></div><div>More indent needed here; line this up with the NewFD in the line above.</div>
<div><br></div><div>+ Diag(D.getIdentifierLoc(), diag::err_image_type_usage) <<</div><div><br></div><div>Put the << on the next line.</div><div><br></div><div>+ ( NewFD->getResultType()->isImageType() ? 0 : 1 ) << 4;</div>
<div><br></div><div>Indent the continuation line. No spaces inside (). Again, please explain what the '4' means.</div><div><br></div><div>+ D.setInvalidType();</div><div><br></div><div>Is this necessary?</div><div>
<br></div></div><div>(Likewise, mutatis mutandis, for the other three changes.)</div><br><div class="gmail_quote">On Tue, Feb 12, 2013 at 1:16 AM, Benyei, Guy <span dir="ltr"><<a href="mailto:guy.benyei@intel.com" target="_blank">guy.benyei@intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div><p class="MsoNormal"><span style="color:#1f497d">Hi All,<u></u><u></u></span></p><p class="MsoNormal"><span style="color:#1f497d">Any comments on this patch?<u></u><u></u></span></p><p class="MsoNormal"><span style="color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:#1f497d">Thanks<u></u><u></u></span></p><p class="MsoNormal"><span style="color:#1f497d"> Guy<u></u><u></u></span></p>
<div><p class="MsoNormal"><span style="color:#1f497d"><span><image001.png></span><u></u><u></u></span></p>
</div><p class="MsoNormal"><span style="color:#1f497d"><u></u> <u></u></span></p>
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in"><p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:cfe-commits-bounces@cs.uiuc.edu" target="_blank">cfe-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:cfe-commits-bounces@cs.uiuc.edu" target="_blank">cfe-commits-bounces@cs.uiuc.edu</a>]
<b>On Behalf Of </b>Benyei, Guy<br>
<b>Sent:</b> Thursday, February 07, 2013 23:22<br>
<b>To:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> [PATCH] OpenCL images and samplers related restriction (6.9.b)<u></u><u></u></span></p>
</div>
</div><div><div class="h5"><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Hi all,<u></u><u></u></p><p class="MsoNormal">Attached the implementation of OpenCL restrictions 6.9.b. It includes image and sampler restrictions.<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Please review.<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Thanks<u></u><u></u></p><p class="MsoNormal"><span><image001.png></span><u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p>
</div></div></div><p>---------------------------------------------------------------------<br>
Intel Israel (74) Limited</p><p>This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</p></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></blockquote></div><br></body></html>