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