[PATCH] D72235: [clang-tidy] new altera unroll loops check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Mostly just nits for the check, otherwise this LGTM.



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:65
+    }
+    if (isa<WhileStmt>(Loop) || isa<DoStmt>(Loop)) {
+      diag(Loop->getBeginLoc(),
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:83-84
+UnrollLoopsCheck::unrollType(const Stmt *Statement, ASTContext *Context) {
+  const clang::DynTypedNodeList Parents = Context->getParents<Stmt>(*Statement);
+  for (const clang::DynTypedNode &Parent : Parents) {
+    const auto *ParentStmt = Parent.get<AttributedStmt>();
----------------
Pretty sure you don't need to use a qualified name here.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:116-120
+  if (isa<CXXForRangeStmt>(Statement)) {
+    if (CXXLoopBound)
+      return true;
+    return false;
+  }
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:123
+  // unrolling for these.
+  if (isa<WhileStmt>(Statement) || isa<DoStmt>(Statement))
+    return false;
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:176-177
+  if (isa<CXXForRangeStmt>(Statement)) {
+    const auto *LoopBoundExpr = dyn_cast<Expr>(CXXLoopBound);
+    return exprHasLargeNumIterations(LoopBoundExpr, Context);
+  }
----------------
The cast isn't necessary (`IntergerLiteral` is already an `Expr`). You should probably assert that `CXXLoopBound` isn't null here though.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:180-181
+  const auto *ForLoop = dyn_cast<ForStmt>(Statement);
+  if (!ForLoop)
+    llvm_unreachable("Unknown loop");
+  const Stmt *Initializer = ForLoop->getInit();
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:196-197
+  }
+  if (!isa<BinaryOperator>(Conditional))
+    llvm_unreachable("Conditional is not a binary operator");
+  int EndValue;
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72235/new/

https://reviews.llvm.org/D72235