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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
