[PATCH] D20734: [clang-format] insert new #includes into correct blocks when cleaning up Replacement with cleanupAroundReplacements().

[Daniel Jasper via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

A few nitpicks, but otherwise looks good.


================
Comment at: lib/Format/Format.cpp:1287
@@ +1286,3 @@
+    int Ret = INT_MAX;
+    for (unsigned I = 0, E = CategoryRegexs.size(); I != E; ++I)
+      if (CategoryRegexs[I].match(IncludeName)) {
----------------
I'd consistently try to use i and e for integers vs. I and E for iterators.

================
Comment at: unittests/Format/CleanupTest.cpp:256
@@ +255,3 @@
+  inline std::string apply(StringRef Code, const tooling::Replacements Replaces,
+                           const FormatStyle &Style = getLLVMStyle()) {
+    return applyAllReplacements(
----------------
I'd probably make "Style" a class member, defaulting to LLVM Style that you can then overwrite for other styles.

================
Comment at: unittests/Format/CleanupTest.cpp:270
@@ +269,3 @@
+
+  int getOffset(StringRef Code, int Line, int Column) {
+    RewriterTestContext Context;
----------------
I'd consider making "Code" a member variable. But it has pros and cons.

================
Comment at: unittests/Format/CleanupTest.cpp:313
@@ +312,3 @@
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+                     "#define __A_H__\n"
----------------
Why are you starting and ending the header guards with a double underscore? I think this is actually reserved for built-in identifiers. Maybe just stick with the LLVM naming scheme?


http://reviews.llvm.org/D20734