lebedev.ri added inline comments.
================ Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:75 + // libcxx implementation of std::experimental::simd requires at least C++11. + if (!Result.Context->getLangOpts().CPlusPlus11) + return; ---------------- timshen wrote: > MaskRay wrote: > > lebedev.ri wrote: > > > Is it reasonable to suggest to use `<experimental/*>`? > > > I would guess it should be `CPlusPlus2a` > > Added a check-specific option `readability-simd-intrinsics.Experiment`. > > > > > > Is it reasonable to suggest to use <experimental/*>? > > I think there are two approaches to proceed: > 1) We just warn the users, but don't suggest <experimental/simd> fixes. > 2) We warn and suggest <experimental/simd> fixes, but only when a flag is > turned on (off by default). The flag documentation should clearly include > "suggest <experimental/simd> API". > > I'm not sure which one fits better. Ok, i should have commented in more words :) I wasn't questioning the use of `<experimental/simd>` I was questioning the whole approach of **defaulting** to issuing the warning as long as that header can be used with **specified** C++ standard. E.g. there is `<experimental/filesystem>`, but there isn't any check to complain on `fopen()`/`fclose()` and friends. (I do agree that this argument is chicken-or-the-egg problem) ================ Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:87 + return; + bool IsVector = Callee->getReturnType()->isVectorType(); + for (const ParmVarDecl *Parm : Callee->parameters()) { ---------------- MaskRay wrote: > lebedev.ri wrote: > > Same here, i *think* something like this would be better? > > ``` > > AST_MATCHER(FunctionDecl, isVectorFunction) { > > bool IsVector = Node.getReturnType()->isVectorType(); > > for (const ParmVarDecl *Parm : Node.parameters()) { > > QualType Type = Parm->getType(); > > if (Type->isPointerType()) > > Type = Type->getPointeeType(); > > if (Type->isVectorType()) > > IsVector = true; > > > > return IsVector; > > } > > ``` > > + > > ``` > > void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) { > > Finder->addMatcher( > > callExpr(callee( > > allOf( > > functionDecl( > > + allOf( > > matchesName("^::(_mm_|_mm256_|_mm512_|vec_)") > > + , isVectorFunction() > > + ) > > , hasDirectCallee() > > ) > > )), > > unless(isExpansionInSystemHeader())) > > .bind("call"), > > this); > > } > > ``` > Thanks! Added isVectorFunction. I guess I may should use unnamed namespace > for AST_MATCHER, but do I also need the unnamed namespace to enclose > TrySuggestPPC and TrySuggestX86? > I may should use unnamed namespace for AST_MATCHER I //think// so. > do I also need the unnamed namespace to enclose TrySuggestPPC and > TrySuggestX86? No, but do make those `static`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42983 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits