[PATCH] D70094: [clang-tidy] new altera ID dependent backward branch check

[Aaron Ballman via Phabricator via cfe-commits]

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