[cfe-commits] [PATCH] C++11 Range-based Loop Converter

[Sean Silva]

================
Comment at: loop-convert/LoopActions.cpp:836
@@ +835,3 @@
+
+  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());
+  // We require that a single array/container be indexed into by LoopVar.
----------------
You seem to have a lot of these bare calls to `std::min()` to compute the `ConfidenceLevel` all over the place. Could you encapsulate this operation and give it a descriptive name?

================
Comment at: loop-convert/LoopMatchers.cpp:17
@@ +16,3 @@
+
+// FIXME: How best to document complicated matcher expressions? They're fairly
+// self-documenting...but there may be some unintuitive parts.
----------------
I think that most of these "matcher factory" functions would benefit from a documentation comment containing:

* A list of bound variable names (or whatever you call the things captured by `id()` and `.bind()`)
* A chunk of example code with the part that would be matched called out in some way (maybe surround it with `@@@`, `|||` or some other marker which will clearly stand out)

Also, for these in particular, I would like to see a brief explanation of how they fit into the larger for-loop conversion process.


================
Comment at: loop-convert/LoopMatchers.cpp:21
@@ +20,3 @@
+StatementMatcher makeArrayLoopMatcher() {
+  static StatementMatcher LHSMatcher =
+  expression(ignoringImpCasts(declarationReference(to(
----------------
Why are these static? (are they generating static constructors... ew...).

================
Comment at: test/loop-convert/Inputs/structures.h:5
@@ +4,3 @@
+extern "C" {
+extern int printf(const char *restrict, ...);
+}
----------------
I don't think it's legal to use `restrict` in C++, even in `extern "C"`.


http://llvm-reviews.chandlerc.com/D19