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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 59053.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- Nits fixed.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -243,13 +243,39 @@
 
 class CleanUpReplacementsTest : public ::testing::Test {
 protected:
-  tooling::Replacement createReplacement(SourceLocation Start, unsigned Length,
- llvm::StringRef ReplacementText) {
-return tooling::Replacement(Context.Sources, Start, Length,
-ReplacementText);
+  tooling::Replacement createReplacement(unsigned Offset, unsigned Length,
+ StringRef Text) {
+return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
-  RewriterTestContext Context;
+  tooling::Replacement createInsertion(StringRef HeaderName) {
+return createReplacement(UINT_MAX, 0, HeaderName);
+  }
+
+  inline std::string apply(StringRef Code,
+   const tooling::Replacements Replaces) {
+return applyAllReplacements(
+Code, cleanupAroundReplacements(Code, Replaces, Style));
+  }
+
+  inline std::string formatAndApply(StringRef Code,
+const tooling::Replacements Replaces) {
+return applyAllReplacements(
+Code,
+formatReplacements(
+Code, cleanupAroundReplacements(Code, Replaces, Style), Style));
+  }
+
+  int getOffset(StringRef Code, int Line, int Column) {
+RewriterTestContext Context;
+FileID ID = Context.createInMemoryFile(FileName, Code);
+auto DecomposedLocation =
+Context.Sources.getDecomposedLoc(Context.getLocation(ID, Line, Column));
+return DecomposedLocation.second;
+  }
+
+  const std::string FileName = "fix.cpp";
+  FormatStyle Style = getLLVMStyle();
 };
 
 TEST_F(CleanUpReplacementsTest, FixOnlyAffectedCodeAfterReplacements) {
@@ -268,17 +294,247 @@
  "namespace D { int i; }\n\n"
  "int x= 0;"
  "}";
-  FileID ID = Context.createInMemoryFile("fix.cpp", Code);
-  tooling::Replacements Replaces;
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 3, 3), 6, ""));
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 9, 34), 6, ""));
-
-  format::FormatStyle Style = format::getLLVMStyle();
-  auto FinalReplaces = formatReplacements(
-  Code, cleanupAroundReplacements(Code, Replaces, Style), Style);
-  EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
+  tooling::Replacements Replaces = {
+  createReplacement(getOffset(Code, 3, 3), 6, ""),
+  createReplacement(getOffset(Code, 9, 34), 6, "")};
+
+  EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  tooling::Replacements Replaces = {createInsertion("#include \"a.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  tooling::Replacements Replaces = {createInsertion("#include \"b.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  tooling::Replacements Replaces = {createInsertion("#include \"a.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+

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

2016-05-31 Thread 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  = 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



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

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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 59043.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- Addressed reviewer's comments.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -243,13 +243,39 @@
 
 class CleanUpReplacementsTest : public ::testing::Test {
 protected:
-  tooling::Replacement createReplacement(SourceLocation Start, unsigned Length,
- llvm::StringRef ReplacementText) {
-return tooling::Replacement(Context.Sources, Start, Length,
-ReplacementText);
+  tooling::Replacement createReplacement(unsigned Offset, unsigned Length,
+ StringRef Text) {
+return tooling::Replacement(FileName, Offset, Length, Text);
   }
 
-  RewriterTestContext Context;
+  tooling::Replacement createInsertion(StringRef HeaderName) {
+return createReplacement(UINT_MAX, 0, HeaderName);
+  }
+
+  inline std::string apply(StringRef Code, const tooling::Replacements Replaces,
+   const FormatStyle  = getLLVMStyle()) {
+return applyAllReplacements(
+Code, cleanupAroundReplacements(Code, Replaces, Style));
+  }
+
+  inline std::string formatAndApply(StringRef Code,
+const tooling::Replacements Replaces,
+const FormatStyle  = getLLVMStyle()) {
+return applyAllReplacements(
+Code,
+formatReplacements(
+Code, cleanupAroundReplacements(Code, Replaces, Style), Style));
+  }
+
+  int getOffset(StringRef Code, int Line, int Column) {
+RewriterTestContext Context;
+FileID ID = Context.createInMemoryFile(FileName, Code);
+auto DecomposedLocation =
+Context.Sources.getDecomposedLoc(Context.getLocation(ID, Line, Column));
+return DecomposedLocation.second;
+  }
+
+  const std::string FileName = "fix.cpp";
 };
 
 TEST_F(CleanUpReplacementsTest, FixOnlyAffectedCodeAfterReplacements) {
@@ -268,17 +294,252 @@
  "namespace D { int i; }\n\n"
  "int x= 0;"
  "}";
-  FileID ID = Context.createInMemoryFile("fix.cpp", Code);
-  tooling::Replacements Replaces;
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 3, 3), 6, ""));
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 9, 34), 6, ""));
-
-  format::FormatStyle Style = format::getLLVMStyle();
-  auto FinalReplaces = formatReplacements(
-  Code, cleanupAroundReplacements(Code, Replaces, Style), Style);
-  EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
+  tooling::Replacements Replaces = {
+  createReplacement(getOffset(Code, 3, 3), 6, ""),
+  createReplacement(getOffset(Code, 9, 34), 6, "")};
+
+  EXPECT_EQ(Expected, formatAndApply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  tooling::Replacements Replaces = {createInsertion("#include \"a.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  tooling::Replacements Replaces = {createInsertion("#include \"b.h\"")};
+  EXPECT_EQ(Expected, apply(Code, Replaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+

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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric added inline comments.


Comment at: unittests/Format/CleanupTest.cpp:310
@@ +309,3 @@
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\"")};

djasper wrote:
> Well, the only reason you seem to be using the FileID or "Context" for that 
> matter is to translate between line/col and offset. I'd just pull that 
> functionality out into a separate function (which does it based on the "Code" 
> input) or not at all (hard-coding the offset doesn't seem *that* bad).
Ohh, I see! Thanks!


Comment at: unittests/Format/CleanupTest.cpp:510
@@ +509,3 @@
+TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
+  std::string Code = "\nint x;";
+  std::string Expected = "#include \"fix.h\"\n"

djasper wrote:
> Have you seen this comment?
Sorry that I missed this one... 


http://reviews.llvm.org/D20734



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

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

2016-05-31 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: unittests/Format/CleanupTest.cpp:310
@@ +309,3 @@
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\"")};

Well, the only reason you seem to be using the FileID or "Context" for that 
matter is to translate between line/col and offset. I'd just pull that 
functionality out into a separate function (which does it based on the "Code" 
input) or not at all (hard-coding the offset doesn't seem *that* bad).


Comment at: unittests/Format/CleanupTest.cpp:510
@@ +509,3 @@
+TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
+  std::string Code = "\nint x;";
+  std::string Expected = "#include \"fix.h\"\n"

Have you seen this comment?


http://reviews.llvm.org/D20734



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

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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 59035.
ioeric marked 7 inline comments as done.
ioeric added a comment.

- Addressed commments.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -269,18 +269,313 @@
  "int x= 0;"
  "}";
   FileID ID = Context.createInMemoryFile("fix.cpp", Code);
-  tooling::Replacements Replaces;
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 3, 3), 6, ""));
-  Replaces.insert(tooling::Replacement(Context.Sources,
-   Context.getLocation(ID, 9, 34), 6, ""));
+  tooling::Replacements Replaces = {
+  tooling::Replacement(Context.Sources, Context.getLocation(ID, 3, 3), 6,
+   ""),
+  tooling::Replacement(Context.Sources, Context.getLocation(ID, 9, 34), 6,
+   "")};
 
   format::FormatStyle Style = format::getLLVMStyle();
   auto FinalReplaces = formatReplacements(
   Code, cleanupAroundReplacements(Code, Replaces, Style), Style);
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\"")};
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\"")};
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\"")};
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces = {
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include ")};
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+

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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric added inline comments.


Comment at: unittests/Format/CleanupTest.cpp:310
@@ +309,3 @@
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;

djasper wrote:
> 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);
> 
> ?
I had tried to pull out the environment setup...but it didn't seem to save much 
code without sacrificing flexibility.

In some test cases, we also need to add replacements other than header 
insertions, and those replacements construction uses `FileID`, so 
`createInMemoryFile` needs to stay in test functions.

`FormatStyle` differs among test cases, so it has to stay.

All we save might just be the two lines which could be either cleanup or 
cleanup+format.\

Any better ideas?


http://reviews.llvm.org/D20734



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

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

2016-05-31 Thread 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  : 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

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

2016-05-31 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 59017.
ioeric added a comment.

- Use std::set_difference in fixCppIncludeInsertions()


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -281,6 +281,331 @@
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include "));
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"z.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterSystemHeaderGoogle) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \n"
+ "#include \"z.h\"\n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+

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

2016-05-31 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: lib/Format/Format.cpp:1549
@@ -1408,3 +1548,3 @@
   // We need to use lambda function here since there are two versions of
   // `cleanup`.
   auto Cleanup = [](const FormatStyle , StringRef Code,

So, add a copy constructor to Replacement?


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 58993.
ioeric added a comment.

- Removed inline from isHeaderInsertion.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -281,6 +281,331 @@
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include "));
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"z.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterSystemHeaderGoogle) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \n"
+ "#include \"z.h\"\n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 58992.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- Addressed reviewer's comments.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -281,6 +281,331 @@
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include "));
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"z.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterSystemHeaderGoogle) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \n"
+ "#include \"z.h\"\n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp",

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric added inline comments.


Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+  HeaderInsertionReplaces =
+  fixCppIncludeInsertions(Code, HeaderInsertionReplaces, Style);
+  NewReplaces.insert(HeaderInsertionReplaces.begin(),

djasper wrote:
> So, not knowing anything of it, what's the difference between 
> fixCppIncludeInsertions and fixInsertionReplacements and when do you need to 
> call what. To me the fact that it's that difficult to find a name for it is 
> telling..
> 
> Also, I still don't understand why you think it is better to separate the two 
> functions. In that way you are currently implementing it, it is certainly 
> less efficient. You are always copying all the replacements, which (in the 
> way you are doing it) is O(N lg N), even if there isn't a single header 
> insertion. Maybe at least use set_difference (similar to what I am showing 
> below).
> 
> So, how I would structure it:
> 
> - Pull out a function isHeaderInsertion(const Replacement &). Easy naming, 
> good to pull this functionality out.
> - At the start of fixCppIncludeInsertions, write:
> 
> 
> ```
> tooling::Replacements HeaderInsertions;
> for (const auto  : Replaces)
>   if (isHeaderInsertion(R))
> HeaderInsertions.insert(R);
> if (HeaderInsertions.empty())
>   return Replaces;
> 
> tooling::Replacements Result;
> std::set_difference(Replaces.begin(), Replaces.end(),
> HeaderInsertions.begin(), HeaderInsertions.end(),
> Result.begin());
> 
> ```  
> - Do what the function currently does with HeaderInsertions and add the 
> results back to Result.
I really appreciate the way you structure it, which does make the code clearer. 

Just one coveat: `std::set_difference()` is not applicable on `Replacements` 
since it calls `std::copy()` on `Replacement`, which `Replacement` does not 
support.

I think another way to approach it would be to delete header insertion 
replacement and insert new replacements back if we can assume that there are 
only few header insertion replacements.  


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: lib/Format/Format.cpp:1284
@@ +1283,3 @@
+  // if \p CheckMainHeader is true and \p IncludeName is a main header, returns
+  // 0. Otherwise, returns INT_MAX if \p IncludeName does not match any 
category
+  // pattern.

Nit: Otherwise, returns the priority of the matching category or INT_MAX.


Comment at: lib/Format/Format.cpp:1503
@@ +1502,3 @@
+  //  - to the beginning of the file.
+  for (auto Iter = ++Priorities.begin(), E = Priorities.end(); Iter != E;
+   ++Iter)

nit: I think you should be using "I" and "E".


Comment at: lib/Format/Format.cpp:1508
@@ +1507,3 @@
+  --Prev;
+  CategoryEndOffsets[*Iter] = CategoryEndOffsets[*Prev];
+}

Can you just use "std::prev(I)" instead of Prev?


Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+  HeaderInsertionReplaces =
+  fixCppIncludeInsertions(Code, HeaderInsertionReplaces, Style);
+  NewReplaces.insert(HeaderInsertionReplaces.begin(),

So, not knowing anything of it, what's the difference between 
fixCppIncludeInsertions and fixInsertionReplacements and when do you need to 
call what. To me the fact that it's that difficult to find a name for it is 
telling..

Also, I still don't understand why you think it is better to separate the two 
functions. In that way you are currently implementing it, it is certainly less 
efficient. You are always copying all the replacements, which (in the way you 
are doing it) is O(N lg N), even if there isn't a single header insertion. 
Maybe at least use set_difference (similar to what I am showing below).

So, how I would structure it:

- Pull out a function isHeaderInsertion(const Replacement &). Easy naming, good 
to pull this functionality out.
- At the start of fixCppIncludeInsertions, write:


```
tooling::Replacements HeaderInsertions;
for (const auto  : Replaces)
  if (isHeaderInsertion(R))
HeaderInsertions.insert(R);
if (HeaderInsertions.empty())
  return Replaces;

tooling::Replacements Result;
std::set_difference(Replaces.begin(), Replaces.end(),
HeaderInsertions.begin(), HeaderInsertions.end(),
Result.begin());

```  
- Do what the function currently does with HeaderInsertions and add the results 
back to Result.


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric added inline comments.


Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+// Insert #include directives into the correct blocks.
+tooling::Replacements fixHeaderInsertions(StringRef Code,
+  const tooling::Replacements 
,

djasper wrote:
> Hm.. Not happy with the naming. There is no real difference between 
> fixHeaderInsertions and fixCppIncludeInsertions. You could argue that the 
> "Cpp" part is the difference, but the actual difference is that one iterates 
> over an entire set of replacements whereas the other only modifies header 
> insertions.
> 
> I think I'd just merge the two functions, but that probably also ties back to 
> my wish not to separate out the two replacement sets ;-).
How about `fixInsertionReplacements`?


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 58982.
ioeric marked 16 inline comments as done.
ioeric added a comment.

- Addressed reviewers' comments.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -281,6 +281,331 @@
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"b.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include "));
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(
+  tooling::Replacement("fix.cpp", UINT_MAX, 0, "#include \"z.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterSystemHeaderGoogle) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \n"
+ "#include \"z.h\"\n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp",

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

2016-05-30 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: include/clang/Format/Format.h:778
@@ -776,1 +777,3 @@
+/// replacement corresponding to the header insertion has offset UINT_MAX (i.e.
+/// -1U).
 tooling::Replacements

Why don't we just stick with UINT_MAX everywhere?


Comment at: lib/Format/Format.cpp:1457
@@ +1456,3 @@
+  IncludeCategoryManager Categories(Style, FileName);
+  int MaxPriority = 0;
+  for (const auto  : Style.IncludeCategories)

Unused


Comment at: lib/Format/Format.cpp:1466
@@ +1465,3 @@
+  // Add 0 for main header and INT_MAX for headers that are not in any 
category.
+  std::vector Priorities = {0, INT_MAX};
+  for (const auto  : Style.IncludeCategories)

Why not just a set?


Comment at: lib/Format/Format.cpp:1488
@@ +1487,3 @@
+Offset += Line.size() + 1;
+// FIXME: make header guard matching more strict, e.g. consider #ifndef.
+if (AfterHeaderGuard == 0 && DefineRegex.match(Line))

nit: s/more strict/stricter/


Comment at: lib/Format/Format.cpp:1494
@@ +1493,3 @@
+  // Set EndOffset of each category that is not set yet to be the end of the
+  // last category with higher priority. If there is no category with higher
+  // priority, then we look for the next block with lower category; if such

Are you sure that this is what's implemented? The way I see it is:
- Ensure that EndOffset[Highest] is always populated.
- If EndOffset[Priority] isn't set, use the next higher value that is set, up 
to EndOffset[Highest].


Comment at: lib/Format/Format.cpp:1513
@@ +1512,3 @@
+} else {
+  // Insert a new line after header guard.
+  // FIXME: since the new line and the end offset are both set to

I'd leave all of this logic out. This only affects headers with no current 
#includes, which is rare to begin with and on top of that, this is more to do 
with whether or not we want to automatically add new lines between the 
different blocks. As it currently is, this would do both:

  #define HEADER_GUARD

  #include  // inserted

and

  #define HEADER_GUARD
  #include  // inserted
  #include 

Which might be the right behavior, but I think this is the wrong place to worry 
about it. In the long run, we should probably make clang-format inserted empty 
lines around the #include blocks if desired.


Comment at: lib/Format/Format.cpp:1525
@@ +1524,3 @@
+  }
+  for (int I = 1, E = Priorities.size(); I != E; I++)
+if (CategoryEndOffsets.find(Priorities[I]) == CategoryEndOffsets.end())

Add a comment:

  // By this point, CategoryEndOffset[Highest] is always set appropriately:
  //  - to an appropriate location before/after existing #includes, or
  //  - to right after the header guard, or
  //  - to the beginning of the file.


Comment at: lib/Format/Format.cpp:1531
@@ +1530,3 @@
+auto IncludeDirective = R.getReplacementText();
+if (!IncludeRegex.match(IncludeDirective, )) {
+  llvm::errs() << IncludeDirective

This should be done together with the check for the offset being -1u. 
Eventually we are going to have something else that we are going to insert 
(e.g. using declarations) and we don't want to confuse those. And even more so, 
if all replacements at offset -1u are not #includes, we don't need to execute 
most of this function.


Comment at: lib/Format/Format.cpp:1538
@@ +1537,3 @@
+int Category =
+Categories.getIncludePriority(IncludeName, /*CheckMainHeader=*/false);
+Offset = CategoryEndOffsets[Category];

Why is CheckMainHeader false?


Comment at: lib/Format/Format.cpp:1541
@@ +1540,3 @@
+std::string NewInclude =
+(!IncludeDirective.endswith("\n"))
+? (IncludeDirective + "\n").str()

No parentheses.


Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+// Insert #include directives into the correct blocks.
+tooling::Replacements fixHeaderInsertions(StringRef Code,
+  const tooling::Replacements 
,

Hm.. Not happy with the naming. There is no real difference between 
fixHeaderInsertions and fixCppIncludeInsertions. You could argue that the "Cpp" 
part is the difference, but the actual difference is that one iterates over an 
entire set of replacements whereas the other only modifies header insertions.

I think I'd just merge the two functions, but that probably also ties back to 
my wish not to separate out the two replacement sets ;-).


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Benjamin Kramer via cfe-commits 
bkramer added inline comments.


Comment at: lib/Format/Format.cpp:1283
@@ +1282,3 @@
+  // Returns the priority of the category which \p IncludeName belongs to, and
+  // if \p CheckMainHeader is true and \p IncldueName is a
+  // main header, returns 0. Otherwise, returns INT_MAX if \p IncludeName does

Typo: IncldueName


Comment at: lib/Format/Format.cpp:1503
@@ +1502,3 @@
+  // we set its EndOffset to be AfterHeaderGuard. With this setup, the category
+  // with the highest priority has been intialized so that all other categories
+  // can find a category with higher priority and initialized EndOffset.

Typo: intialized


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric added inline comments.


Comment at: lib/Format/Format.cpp:1551
@@ +1550,3 @@
+return Replaces;
+  tooling::Replacements HeaderInsertionReplaces;
+  tooling::Replacements NewReplaces;

djasper wrote:
> Why do you split out all the header insertion replacements here instead of 
> just ignoring the ones that aren't header insertions in 
> fixCppIncludeInsertions?
I think this enables us to exit early in `fixCppIncludeInsertions` if there is 
no header insertion replacement at all. Also, it is easier to split out header 
insertion replacements and then merge fixed replacements with other 
replacements instead of doing this in place, which IMO might not be more 
efficient since there are still set deletion and insertion for each header 
insertion replacement.


http://reviews.llvm.org/D20734



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

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

2016-05-30 Thread Eric Liu via cfe-commits 
ioeric updated this revision to Diff 58952.
ioeric marked 9 inline comments as done.
ioeric added a comment.

- Addressed reviewer's comments.


http://reviews.llvm.org/D20734

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp

Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -281,6 +281,323 @@
   EXPECT_EQ(Expected, applyAllReplacements(Code, FinalReplaces));
 }
 
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", -1U, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", -1U, 0, "#include \"b.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef __A_H__\n"
+ "#define __A_H__\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", -1U, 0, "#include \"a.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", -1U, 0, "#include "));
+  format::FormatStyle Style =
+  format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp);
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements Replaces;
+  Replaces.insert(tooling::Replacement("fix.cpp", -1U, 0, "#include \"z.h\""));
+  format::FormatStyle Style = format::getLLVMStyle();
+  auto NewReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  EXPECT_EQ(Expected, applyAllReplacements(Code, NewReplaces));
+}
+
+TEST_F(CleanUpReplacementsTest, InsertAfterSystemHeaderGoogle) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \n"
+ "#include \"z.h\"\n"
+ "\n"
+ "int main() {}";
+  Context.createInMemoryFile("fix.cpp", Code);
+  tooling::Replacements

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

2016-05-29 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: lib/Format/Format.cpp:1411
@@ +1410,3 @@
+int getIncludePriority(const FormatStyle ,
+   SmallVector ,
+   StringRef IncludeName) {

But you should pass it as an ArrayRef or a SmallVectorImpl.


Comment at: lib/Format/Format.cpp:1413
@@ +1412,3 @@
+   StringRef IncludeName) {
+  for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+if (CategoryRegexs[i].match(IncludeName)) {

nit: no braces


Comment at: lib/Format/Format.cpp:1418
@@ +1417,3 @@
+  }
+  return -1;
+}

From the docs in Format.h: "If none of the regular expressions match, INT_MAX 
is assigned". If that is true for an existing #include as well as the inserted 
one, I don't see a reason, why those should belong to the same block. So, I 
think you should return INT_MAX here and remove all the special-casing below.


Comment at: lib/Format/Format.cpp:1431
@@ +1430,3 @@
+
+  llvm::Regex IncludeRegex(
+  R"(^[\t\ ]*#[\t\ ]*include[\t\ ]*(["<][^">]*[">]))");

We already have an include regex in this file, I don't think we should have 
two. Can you pull out the string into a constant?


Comment at: lib/Format/Format.cpp:1435
@@ +1434,3 @@
+  SmallVector Matches;
+  // Create pre-compiled regular expressions for the #include categories.
+  SmallVector CategoryRegexs;

There is a lot of duplicated code between this and sortCppIncludes. Pull common 
code out into functions.


Comment at: lib/Format/Format.cpp:1454
@@ +1453,3 @@
+  // extra category (priority 0) for main include.
+  std::vector CategoryEndOffsets(MaxPriority + 1, -1);
+  bool MainIncludeFound = false;

There is no guarantee that people assign categories sequentially, I can easily 
see people assigning category "1000" or "1" to be able to add more 
categories later. I think this should be a map and I think you then also don't 
really need the vector. Also note that categories can be negative, which I 
think would break this.


Comment at: lib/Format/Format.cpp:1458
@@ +1457,3 @@
+  int Offset = 0;
+  int AfterFirstDefine = 0;
+  SmallVector Lines;

I am guessing that AfterFirstDefine is used so that we insert #includes after 
header guards. I think we should name it AfterHeaderGuard than and either make 
the behavior more strict or at least add a FIXME for it (only in header files, 
only if the #define is after an #ifndef, ...)


Comment at: lib/Format/Format.cpp:1482
@@ +1481,3 @@
+  } else {
+CategoryEndOffsets[Category] = Offset + Line.size() + 1;
+  }

I think if you assign the EndOffset of all lower categories that aren't set 
yet, that will simplify the logic below.


Comment at: lib/Format/Format.cpp:1509
@@ +1508,3 @@
+// Append the new #include after the corresponding category if the category
+// exists. Otherwise, append after the last category that has higher
+// priority than the current category. If there is no category, insert 
after

What happens if there is no category with higher priority, but there is a 
category with lower priority. Shouldn't you then insert the #include right 
before that?


Comment at: lib/Format/Format.cpp:1548
@@ +1547,3 @@
+  const FormatStyle ) {
+  // FIXME: support other languages.
+  if (Style.Language != FormatStyle::LanguageKind::LK_Cpp)

I think you can remove this FIXME.. Seems obvious.


Comment at: lib/Format/Format.cpp:1551
@@ +1550,3 @@
+return Replaces;
+  tooling::Replacements HeaderInsertionReplaces;
+  tooling::Replacements NewReplaces;

Why do you split out all the header insertion replacements here instead of just 
ignoring the ones that aren't header insertions in fixCppIncludeInsertions?


Comment at: lib/Format/Format.cpp:1554
@@ +1553,3 @@
+  for (const auto  : Replaces) {
+if (R.getOffset() == -1U) {
+  HeaderInsertionReplaces.insert(R);

nit: No braces.


http://reviews.llvm.org/D20734



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