[PATCH] D17031: Control Structure Analysis

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

I just skimmed through it and made very quick comments, I haven't tried it yet.
Also, can you unittest this?


================
Comment at: include/llvm/Analysis/ControlStructureAnalysis.h:1
@@ +1,2 @@
+//===- llvm/Analysis/ControlStructureAnalysis.h - Control Structure Analysis --*- C++ -*-===//
+//
----------------
This line looks to long, I guess you should run clang-format on the rest of the code as well.

Also please adapt the naming for the the LLVM coding conventions.

================
Comment at: lib/Analysis/ControlStructureAnalysis.cpp:582
@@ +581,3 @@
+  assert(preOrderCtr == postOrderCtr);
+}
+
----------------
I'm not just what is the justification for the goats here, refactoring / restructuring shouldn't be out-of-reach.

================
Comment at: lib/Analysis/ControlStructureAnalysis.cpp:603
@@ +602,3 @@
+    nset.push_back(n);
+  // We want this in the reverse order (lexicogrphaically).
+  std::reverse(nset.begin(), nset.end());
----------------
s/lexicogrphaically/lexicographically/

================
Comment at: lib/Analysis/ControlStructureAnalysis.cpp:1145
@@ +1144,3 @@
+          entryNode = p;
+      } else {
+        // Locate a cyclic region, if present.
----------------
I think adding a continue just before this line would save the `else` and reduce the indentation level.


http://reviews.llvm.org/D17031