[PATCH] D20795: Added ASTStructure for analyzing the structure of Stmts.

[Vassil Vassilev via cfe-commits]

v.g.vassilev requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/clang/Basic/SourceManager.h:1173
@@ +1172,3 @@
+
+  //
+
----------------
A leftover from something else?

================
Comment at: include/clang/Basic/SourceManager.h:1179
@@ +1178,3 @@
+  /// \returns Returns true if the SourceLocation is expanded from any macro
+  /// body. Returns false if the SourceLocation is invalid, is from not in a
+  /// macro expansion, or is from expanded from a top-level macro argument.
----------------
Could you revise the grammar of this entire sentence?

================
Comment at: lib/AST/ASTStructure.cpp:42
@@ +41,3 @@
+    // At this point we know that all Visit* calls for the previous Stmt have
+    // finished, so we know save the calculated hash before starting
+    // to calculate the next hash.
----------------
You meant "now" instead of "know"?

================
Comment at: lib/AST/ASTStructure.cpp:88
@@ +87,3 @@
+    default:
+      return Context.getSourceManager().IsInAnyMacroBody(S->getLocStart()) ||
+             Context.getSourceManager().IsInAnyMacroBody(S->getLocEnd());
----------------
I'd add the SourceManager as a member and shorten this code snippet.

================
Comment at: lib/AST/ASTStructure.cpp:93
@@ +92,3 @@
+
+  // Marks the current Stmt as no to be processed.
+  // Always returns \c true that it can be called
----------------
You meant "as not to be"?

================
Comment at: lib/AST/ASTStructure.cpp:100
@@ +99,3 @@
+
+  // Merges the given value into the current hash code.
+  void CalcHash(unsigned Value) {
----------------
Is that the LLVM/Clang common notion for documenting private members: // (i.e. doxygen disabled) instead of ///

================
Comment at: lib/AST/ASTStructure.cpp:146
@@ +145,3 @@
+  // All members specify properties of the hash process for the current
+  // Stmt. They are resetted after the Stmt is successfully hased.
+
----------------
Why this is dangling? Also "reset".

================
Comment at: lib/AST/ASTStructure.cpp:202
@@ +201,3 @@
+// invalid code.
+DEF_STMT_VISIT(TypoExpr, { return Skip(); })
+DEF_STMT_VISIT(UnresolvedLookupExpr, { return Skip(); })
----------------
Maybe we should issue a warning about it.

================
Comment at: lib/AST/ASTStructure.cpp:366
@@ +365,3 @@
+  auto numDecls = std::distance(S->decl_begin(), S->decl_end());
+  CalcHash(537u + static_cast<unsigned>(numDecls));
+})
----------------
Could we document these "magic" prime numbers and move them in a central place. I think this would make it more readable.

================
Comment at: lib/AST/ASTStructure.cpp:404
@@ +403,3 @@
+// TODO: implement hashing custom data of these Stmts
+DEF_STMT_VISIT(AsmStmt, {})
+DEF_STMT_VISIT(MSAsmStmt, {})
----------------
Could the implementation of GCCAsmStmt go here in the base class? And GCCAsmStmt to fall back to the base class (I assume this way we would have basic support for MSAsmStmt). This holds for the other cases of this pattern, eg.: *Literal.

================
Comment at: unittests/AST/ASTStructureTest.cpp:46
@@ +45,3 @@
+
+  auto Hash = ASTStructure(ASTUnit->getASTContext());
+
----------------
Why not passing just the ASTContext variable?

================
Comment at: unittests/AST/ASTStructureTest.cpp:80
@@ +79,3 @@
+
+  auto Hash = ASTStructure(ASTUnit->getASTContext());
+
----------------
Same copy-paste here ;) See above.

================
Comment at: unittests/AST/ASTStructureTest.cpp:103
@@ +102,3 @@
+  auto &ASTContextA = ASTUnitA->getASTContext();
+  auto &ASTContextB = ASTUnitB->getASTContext();
+
----------------
I'd move this up and reuse the variables in the rhs of HashA and HashB.

================
Comment at: unittests/AST/ASTStructureTest.cpp:130
@@ +129,3 @@
+  ASSERT_FALSE(compareStmt("if (true) { int x; }", "if (false) {}"));
+  ASSERT_FALSE(compareStmt("if (int y = 0) {}", "if (false) {}"));
+}
----------------
What we will get from compareStmt("if (1 == 2) {}", "if (false) {}")


http://reviews.llvm.org/D20795