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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
