[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

[Roman Lebedev via Phabricator via cfe-commits]

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