[PATCH] D143849: [Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+      // of SVM.
+      if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+          (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
----------------
Ayal wrote:
> Anastasia wrote:
> > I think it should be possible to merge this with `if` below to avoid condition duplication.
> > 
> > 
> Sure, but that trades one duplication for another, rather than clearly separating the early-continue case early?
> 
> ```
>       if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
>         if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
>           continue;
>         S.Diag(Param->getLocation(),
>                diag::err_record_with_pointers_kernel_param)
>           << PT->isUnionType()
>           << PT;
>       } else if (ParamType == InvalidAddrSpacePtrKernelParam) {
>         S.Diag(Param->getLocation(),
>                diag::err_record_with_pointers_kernel_param)
>           << PT->isUnionType()
>           << PT;
>       } else {
>         S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
> 
> ```
I am mainly thinking in terms of maintenance if for example someone fixes one if and forgets another. Or if ifs will get separated by some other code and then it is not easy to see that the same thing is handled differently in OpenCL versions. 

Unfortunately we have a lot of those cases, I know this function has early exists but it is not a common style.


I was talking about something like:


```
if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
    (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
          ParamType == InvalidAddrSpacePtrKernelParam)
```

It would also be ok to separate `InvalidAddrSpacePtrKernelParam` since it's handling different feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143849