Anastasia added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:9494
+ // of SVM.
+ if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
+ (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
----------------
Anastasia wrote:
> 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.
Sorry I have forgotten that this part of the code is expected to handle the
diagnostics only. The decision that the kernel parameter is wrong is done in
`getOpenCLKernelParameterType`. I think you should alter the conditions there
to check for OpenCL version and avoid classifying cases you care about as
`PtrKernelParam` or `PtrPtrKernelParam`. Then here you wouldn't need this extra
if/continue block.
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
