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

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: lib/Format/Format.cpp:1444
@@ +1443,3 @@
+  if (!llvm::Regex(IncludeRegexPattern).match(Replace.getReplacementText())) {
+    llvm::errs() << "Insertions other than header #include insertion are "
+                    "not supported! "
----------------
This error output doesn't belong here. I think we can just remove it. If you'd 
prefer to keep it, add it to the loop in fixCppIncludeInsertions():

  for (const auto &R : Replaces) {
    if (isHeaderInsertion(R))
      HeaderInsertions.insert(R);
    else if (R.getOffset() == UINT_MAX)
      log::errs() << ...
  }

================
Comment at: lib/Format/Format.cpp:1556
@@ +1555,3 @@
+  tooling::Replacements NewReplaces =
+      (Style.Language != FormatStyle::LanguageKind::LK_Cpp)
+          ? Replaces
----------------
I'd move this check into fixCppIncludeInsertions.

================
Comment at: unittests/Format/CleanupTest.cpp:310
@@ +309,3 @@
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
----------------
I'd pull out a lot of these environment setup things into abstractions in the 
test fixture. Maybe all you need is two functions like:

  insert(Code, Replaces);

and

  insertAndFormat(Code, Replaces);

?

================
Comment at: unittests/Format/CleanupTest.cpp:311
@@ +310,3 @@
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
----------------
Can you just make this:

  tooling::Replacements Replaces = {
      tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\"")};

?

================
Comment at: unittests/Format/CleanupTest.cpp:510
@@ +509,3 @@
+  Replaces.insert(
+      tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  Replaces.insert(
----------------
I'd create a function:

  tooling::Replacement createInsertion(StringRef HeaderName) {
    return tooling::Replacement("fix.cpp", UINT_MAX, 0, HeaderName);
  }


http://reviews.llvm.org/D20734



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits