aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:28-31 + hasOperatorName("="), hasOperatorName("*="), hasOperatorName("/="), + hasOperatorName("%="), hasOperatorName("+="), hasOperatorName("-="), + hasOperatorName("<<="), hasOperatorName(">>="), hasOperatorName("&="), + hasOperatorName("^="), hasOperatorName("|=")); ---------------- Can you use the `isAssignmentOperator()` AST matcher instead of spelling out the operator names manually? ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:54 + // Bind all VarDecls that include an initializer with a variable DeclRefExpr + // (incase it is ID-dependent). + Finder->addMatcher( ---------------- ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:101-102 + if (const auto *ChildExpression = dyn_cast<Expr>(Child)) { + IdDependencyRecord *Result = hasIdDepVar(ChildExpression); + if (Result) + return Result; ---------------- ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:121-122 + if (const auto *ChildExpression = dyn_cast<Expr>(Child)) { + IdDependencyRecord *Result = hasIdDepField(ChildExpression); + if (Result) + return Result; ---------------- ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:132-134 + std::ostringstream StringStream; + StringStream << "assignment of ID-dependent variable " + << Variable->getNameAsString(); ---------------- You should use an `llvm::Twine` for this. ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:141-143 + std::ostringstream StringStream; + StringStream << "assignment of ID-dependent field " + << Field->getNameAsString(); ---------------- Same here as above. ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:154 + return; + std::ostringstream StringStream; + StringStream << "inferred assignment of ID-dependent value from " ---------------- I'd use `llvm::raw_string_ostream` here (basically, try to avoid STL iostream functionality as much as possible, it's really slow/bad). ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.cpp:199-204 + if (isa<DoStmt>(Loop)) + return DO_LOOP; + else if (isa<WhileStmt>(Loop)) + return WHILE_LOOP; + else if (isa<ForStmt>(Loop)) + return FOR_LOOP; ---------------- Rather than repeatedly use `isa<>`, I'd consider using a `switch` over `Loop->getStmtClass()`. This also addresses the "no `else` after a `return`" coding style concern. ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h:26 +private: + enum LoopType { UNK_LOOP = -1, DO_LOOP = 0, WHILE_LOOP = 1, FOR_LOOP = 2 }; + // Stores information necessary for printing out source of error. ---------------- The enumerators don't match the style guide -- can you rename them to `UnknownLoop`, `DoLoop`, etc so they look less like macros? ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h:32 + : VariableDeclaration(Declaration), Location(Location), + Message(Message) {} + IdDependencyRecord(const FieldDecl *Declaration, SourceLocation Location, ---------------- ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h:35 + std::string Message) + : FieldDeclaration(Declaration), Location(Location), Message(Message) {} + IdDependencyRecord() = default; ---------------- ================ Comment at: clang-tools-extra/clang-tidy/altera/IdDependentBackwardBranchCheck.h:37-38 + IdDependencyRecord() = default; + const VarDecl *VariableDeclaration; + const FieldDecl *FieldDeclaration; + SourceLocation Location; ---------------- To ensure they get initialized to something sensible from the default ctor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70094/new/ https://reviews.llvm.org/D70094 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits