[PATCH] D104915: [OpenCL] Add support of __opencl_c_read_write_images feature macro

[Anton Zabaznov via Phabricator via cfe-commits]

azabaznov added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7412
+            !S.getOpenCLOptions().isSupported("__opencl_c_read_write_images",
+                                              S.getLangOpts()))) ||
           DeclTy->isPipeType()) {
----------------
thakis wrote:
> Are the parens right here? You probably want
> 
>   `!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200 || (Ver == 300 && isSupported))`
> 
> for the first term, but you have
> 
>   `!S.getLangOpts().OpenCLCPlusPlus && (Ver < 200) || (Ver == 300 && isSupported)`
> 
> Which means the OpenCLCPlusPlus only matters for `(Ver < 200)` and the `(Ver == 300 && ...)` term makes things true independent of the OpenCLCPlusPlus check.
> 
> If what you currently have is what you want: Remove the parens around `(S.getLangOpts().OpenCLVersion < 200)` and do instead parenthesize like so: `(!S.getLangOpts().OpenCLCPlusPlus && Ver < 200)`.
Thanks. I've submitted the patch: https://reviews.llvm.org/D105892. Please approve.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104915/new/

https://reviews.llvm.org/D104915