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

[Nathan James via Phabricator via llvm-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:22
+void UnrollLoopsCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ANYLOOP = anyOf(forStmt(), whileStmt(), doStmt());
+  Finder->addMatcher(stmt(allOf(ANYLOOP, // Match all loop types,
----------------
Dont use all caps variable names, preferred style is CamelCase.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:24-26
+                                unless(hasDescendant(forStmt())),
+                                unless(hasDescendant(whileStmt())),
+                                unless(hasDescendant(doStmt()))))
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:35
+  UnrollType Unroll = unrollType(MatchedLoop, Result.Context);
+  if (Unroll == NotUnrolled) {
+    diag(MatchedLoop->getBeginLoc(),
----------------
This loops like it should be a switch


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:68
+    for (const Attr *Attribute : ParentStmt->getAttrs()) {
+      const auto *LoopHint = static_cast<const LoopHintAttr *>(Attribute);
+      if (LoopHint) {
----------------
Shouldn't this by dyn_cast too.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:69
+      const auto *LoopHint = static_cast<const LoopHintAttr *>(Attribute);
+      if (LoopHint) {
+        switch (LoopHint->getState()) {
----------------
To reduce indentations can this be an early exit


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:79
+          return FullyUnrolled;
+        default:
+          return NotUnrolled;
----------------
Default statements are usually bad as the prevent compiler diagnostics if the enum is ever updated.


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:93-94
+    return false;
+  if (isa<BinaryOperator>(Conditional)) {
+    const auto *BinaryOp = static_cast<const BinaryOperator *>(Conditional);
+    const Expr *LHS = BinaryOp->getLHS();
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:107-120
+  const Expr *Conditional;
+  if (isa<ForStmt>(Statement)) {
+    const auto *ForLoop = static_cast<const ForStmt *>(Statement);
+    Conditional = ForLoop->getCond();
+  }
+  if (isa<WhileStmt>(Statement)) {
+    const auto *WhileLoop = static_cast<const WhileStmt *>(Statement);
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:128
+    return false;
+  if (isa<BinaryOperator>(Conditional)) {
+    const auto *BinaryOp = static_cast<const BinaryOperator *>(Conditional);
----------------
dyn_cast again


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:145
+  if (Expression->EvaluateAsRValue(Result, *Context)) {
+    if (!(Result.Val.isInt()))
+      return false; // Cannot check number of iterations, return false to be
----------------
Elide inner parens


================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp:148-150
+    if (Result.Val.getInt() > MaxLoopIterations)
+      return true; // Assumes values go from 0 to Val in increments of 1.
+    return false; // Number of iterations likely less than MaxLoopIterations.
----------------



================
Comment at: clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.h:31-33
+  UnrollLoopsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        MaxLoopIterations(Options.get("MaxLoopIterations", 100U)) {}
----------------
Should probably define this out of line in the cpp file.


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

https://reviews.llvm.org/D72235