[PATCH] D16623: Factor out UnrollAnalyzer to Analysis, and add unit tests for it.

Sanjoy Das via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 26 22:04:47 PST 2016

sanjoy requested changes to this revision.
sanjoy added a reviewer: sanjoy.
sanjoy added a comment.
This revision now requires changes to proceed.

I think this change requires some reorganization before it can go in.  Please take a look at http://llvm.org/docs/CodingStandards.html

Comment at: include/llvm/Analysis/LoopUnrollAnalyzer.h:6
@@ +5,3 @@
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/raw_ostream.h"
This is very unlike headers in the rest of LLVM -- have you looked at other headers in this directory?  For instance:

 - Headers should not open namespaces
 - License header is missing
 - Function bodies (e.g. `visitBinaryOperator`) should be in the cpp file

Comment at: include/llvm/Analysis/LoopUnrollAnalyzer.h:25
@@ +24,3 @@
+//   v = b[1]
+namespace {
+class UnrolledInstAnalyzer : private InstVisitor<UnrolledInstAnalyzer, bool> {
This should be in namespace llvm.

I'd say it is better to not even expose this as an `InstVisitor` but keep that an implementation detail; but that's debatable.


More information about the llvm-commits mailing list