[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:26
+
+  static const llvm::StringMap<std::string> Mapping{
+    // [simd.alg]
----------------
You probably want to move `Mapping` out of the function.


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:34
+    {"sub", "operator- on std::experimental::simd objects"},
+    {"mul", "operator* on std::experimental::simd objects"},
+  };
----------------
To point the obvious, this is not exhaustive list, at least `div` is missing.


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:55
+  // [simd.binary]
+  if (Name.startswith("add_"))
+    return "operator+ on std::experimental::simd objects";
----------------
This function was not updated to use the `Mapping` map.


================
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;
----------------
Is it reasonable to suggest to use `<experimental/*>`?
I would guess it should be `CPlusPlus2a`


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:84
+  // SIMD intrinsic call.
+  const FunctionDecl *Callee = Call->getDirectCallee();
+  if (!Callee)
----------------
I would refactor this as astmatcher, at least a local one, e.g. something like
```
AST_MATCHER(CallExpr, hasDirectCallee) {
  return Node.getDirectCallee();
}
```
+
```
void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) {
  Finder->addMatcher(
      callExpr(callee(
+            allOf(
                     functionDecl(matchesName("^::(_mm_|_mm256_|_mm512_|vec_)")
+                  , hasDirectCallee()
+            )
               )),
               unless(isExpansionInSystemHeader()))
          .bind("call"),
      this);
}
```
Unless of course there is already a narrower that does that


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:87
+    return;
+  bool IsVector = Callee->getReturnType()->isVectorType();
+  for (const ParmVarDecl *Parm : Callee->parameters()) {
----------------
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);
}
```


================
Comment at: docs/clang-tidy/checks/readability-simd-intrinsics.rst:1
+.. title:: clang-tidy - simd-readability-intrinsics
+
----------------
This should be 
```
.. title:: clang-tidy - readability-simd-intrinsics
```


================
Comment at: docs/clang-tidy/checks/readability-simd-intrinsics.rst:3
+
+simd-readability-intrinsics
+===========================
----------------
Here too


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983