Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-21 Thread Haojian Wu via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL276280: [include-fixer] Add mising qualifiers to all 
instances of an unidentified… (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D22567?vs=64849=64877#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D22567

Files:
  clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp
  clang-tools-extra/trunk/include-fixer/IncludeFixer.h
  clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp
  clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h
  clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
  clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py
  clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp
  clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp

Index: clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp
===
--- clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp
+++ clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp
@@ -1,9 +1,9 @@
 // REQUIRES: shell
 // RUN: echo "foo f;" > %t.cpp
 // RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
-// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
+// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
 //
 // CHECK: "HeaderInfos": [
 // CHECK-NEXT:  {"Header": "\"foo.h\"",
Index: clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp
===
--- clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp
+++ clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp
@@ -89,17 +89,14 @@
   runOnCode(, Code, FakeFileName, ExtraArgs);
   if (FixerContext.getHeaderInfos().empty())
 return Code;
-  auto Replaces = clang::include_fixer::createInsertHeaderReplacements(
-  Code, FakeFileName, FixerContext.getHeaderInfos().front().Header);
+  auto Replaces = clang::include_fixer::createIncludeFixerReplacements(
+  Code, FakeFileName, FixerContext);
   EXPECT_TRUE(static_cast(Replaces))
   << llvm::toString(Replaces.takeError()) << "\n";
   if (!Replaces)
 return "";
   clang::RewriterTestContext Context;
   clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
-  Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(),
-FixerContext.getSymbolRange().getLength(),
-FixerContext.getHeaderInfos().front().QualifiedName});
   clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
@@ -211,12 +208,12 @@
   std::string Code = "#include \"a.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   std::string Expected = "#include \"a.h\"\n"
  "#include \"bar.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
@@ -275,6 +272,69 @@
 runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n"));
 }
 
+TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) {
+  const char TestCode[] = R"(
+namespace a {
+bar b;
+int func1() {
+  bar a;
+

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-21 Thread Benjamin Kramer via cfe-commits 
bkramer accepted this revision.
bkramer added a comment.
This revision is now accepted and ready to land.

lgtm


https://reviews.llvm.org/D22567



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

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-21 Thread Haojian Wu via cfe-commits 
hokein updated this revision to Diff 64849.
hokein added a comment.

update error message.


https://reviews.llvm.org/D22567

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/IncludeFixer.h
  include-fixer/IncludeFixerContext.cpp
  include-fixer/IncludeFixerContext.h
  include-fixer/tool/ClangIncludeFixer.cpp
  include-fixer/tool/clang-include-fixer.py
  test/include-fixer/commandline_options.cpp
  unittests/include-fixer/IncludeFixerTest.cpp

Index: unittests/include-fixer/IncludeFixerTest.cpp
===
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -89,17 +89,14 @@
   runOnCode(, Code, FakeFileName, ExtraArgs);
   if (FixerContext.getHeaderInfos().empty())
 return Code;
-  auto Replaces = clang::include_fixer::createInsertHeaderReplacements(
-  Code, FakeFileName, FixerContext.getHeaderInfos().front().Header);
+  auto Replaces = clang::include_fixer::createIncludeFixerReplacements(
+  Code, FakeFileName, FixerContext);
   EXPECT_TRUE(static_cast(Replaces))
   << llvm::toString(Replaces.takeError()) << "\n";
   if (!Replaces)
 return "";
   clang::RewriterTestContext Context;
   clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
-  Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(),
-FixerContext.getSymbolRange().getLength(),
-FixerContext.getHeaderInfos().front().QualifiedName});
   clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
@@ -211,12 +208,12 @@
   std::string Code = "#include \"a.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   std::string Expected = "#include \"a.h\"\n"
  "#include \"bar.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
@@ -275,6 +272,69 @@
 runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n"));
 }
 
+TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) {
+  const char TestCode[] = R"(
+namespace a {
+bar b;
+int func1() {
+  bar a;
+ bar *p = new bar();
+  return 0;
+}
+} // namespace a
+
+namespace a {
+bar func2() {
+  bar f;
+  return f;
+}
+} // namespace a
+
+// Non-fixed cases:
+void f() {
+  bar b;
+}
+
+namespace a {
+namespace c {
+  bar b;
+} // namespace c
+} // namespace a
+)";
+
+  const char ExpectedCode[] = R"(
+#include "bar.h"
+namespace a {
+b::bar b;
+int func1() {
+  b::bar a;
+  b::bar *p = new b::bar();
+  return 0;
+}
+} // namespace a
+
+namespace a {
+b::bar func2() {
+  b::bar f;
+  return f;
+}
+} // namespace a
+
+// Non-fixed cases:
+void f() {
+  bar b;
+}
+
+namespace a {
+namespace c {
+  bar b;
+} // namespace c
+} // namespace a
+)";
+
+  EXPECT_EQ(ExpectedCode, runIncludeFixer(TestCode));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang
Index: test/include-fixer/commandline_options.cpp
===
--- test/include-fixer/commandline_options.cpp
+++ test/include-fixer/commandline_options.cpp
@@ -1,9 +1,9 @@
 // REQUIRES: shell
 // RUN: echo "foo f;" > %t.cpp
 // RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
-// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
+// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-21 Thread Haojian Wu via cfe-commits 
hokein added inline comments.


Comment at: include-fixer/tool/ClangIncludeFixer.cpp:365
@@ +364,3 @@
+  for (const auto  : Context.getQuerySymbolInfos()) {
+Replacements->insert({FilePath, Info.Range.getOffset(),
+  Info.Range.getLength(),

ioeric wrote:
> Now that we insert namespace qualifiers, we might want to do a 
> `formatReplacements` before applying them.
Good point! So that we won't break the current code style. The line probably 
exceeds 80 characters after adding qualifiers.


https://reviews.llvm.org/D22567



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

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-21 Thread Haojian Wu via cfe-commits 
hokein updated this revision to Diff 64848.
hokein marked 3 inline comments as done.
hokein added a comment.

Address review comments.


https://reviews.llvm.org/D22567

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/IncludeFixer.h
  include-fixer/IncludeFixerContext.cpp
  include-fixer/IncludeFixerContext.h
  include-fixer/tool/ClangIncludeFixer.cpp
  include-fixer/tool/clang-include-fixer.py
  test/include-fixer/commandline_options.cpp
  unittests/include-fixer/IncludeFixerTest.cpp

Index: unittests/include-fixer/IncludeFixerTest.cpp
===
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -89,17 +89,14 @@
   runOnCode(, Code, FakeFileName, ExtraArgs);
   if (FixerContext.getHeaderInfos().empty())
 return Code;
-  auto Replaces = clang::include_fixer::createInsertHeaderReplacements(
-  Code, FakeFileName, FixerContext.getHeaderInfos().front().Header);
+  auto Replaces = clang::include_fixer::createIncludeFixerReplacements(
+  Code, FakeFileName, FixerContext);
   EXPECT_TRUE(static_cast(Replaces))
   << llvm::toString(Replaces.takeError()) << "\n";
   if (!Replaces)
 return "";
   clang::RewriterTestContext Context;
   clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
-  Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(),
-FixerContext.getSymbolRange().getLength(),
-FixerContext.getHeaderInfos().front().QualifiedName});
   clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
@@ -211,12 +208,12 @@
   std::string Code = "#include \"a.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   std::string Expected = "#include \"a.h\"\n"
  "#include \"bar.h\"\n"
  "#include \"foo.h\"\n"
  "\n"
- "namespace a { b::bar b; }";
+ "namespace a {\nb::bar b;\n}\n";
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
@@ -275,6 +272,69 @@
 runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n"));
 }
 
+TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) {
+  const char TestCode[] = R"(
+namespace a {
+bar b;
+int func1() {
+  bar a;
+ bar *p = new bar();
+  return 0;
+}
+} // namespace a
+
+namespace a {
+bar func2() {
+  bar f;
+  return f;
+}
+} // namespace a
+
+// Non-fixed cases:
+void f() {
+  bar b;
+}
+
+namespace a {
+namespace c {
+  bar b;
+} // namespace c
+} // namespace a
+)";
+
+  const char ExpectedCode[] = R"(
+#include "bar.h"
+namespace a {
+b::bar b;
+int func1() {
+  b::bar a;
+  b::bar *p = new b::bar();
+  return 0;
+}
+} // namespace a
+
+namespace a {
+b::bar func2() {
+  b::bar f;
+  return f;
+}
+} // namespace a
+
+// Non-fixed cases:
+void f() {
+  bar b;
+}
+
+namespace a {
+namespace c {
+  bar b;
+} // namespace c
+} // namespace a
+)";
+
+  EXPECT_EQ(ExpectedCode, runIncludeFixer(TestCode));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang
Index: test/include-fixer/commandline_options.cpp
===
--- test/include-fixer/commandline_options.cpp
+++ test/include-fixer/commandline_options.cpp
@@ -1,9 +1,9 @@
 // REQUIRES: shell
 // RUN: echo "foo f;" > %t.cpp
 // RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
-// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
+// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
+// RUN: cat %t.cpp |

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-20 Thread Benjamin Kramer via cfe-commits 
bkramer added inline comments.


Comment at: include-fixer/IncludeFixer.cpp:246
@@ +245,3 @@
+if (!QuerySymbolInfos.empty()) {
+  if (ScopedQualifiers.str() == QuerySymbolInfos.front().ScopedQualifiers 
&&
+  Query.str() == QuerySymbolInfos.front().RawIdentifier) {

Drop .str() when comparing StringRef with std::string.


Comment at: include-fixer/IncludeFixerContext.h:73
@@ -64,1 +72,3 @@
 
+  /// \brief The information of the symbol being queried.
+  std::vector QuerySymbolInfos;

This comment is rather useless as-is, maybe explain what it means to have 
multiple elements in the vector.


https://reviews.llvm.org/D22567



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

Re: [PATCH] D22567: [include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

2016-07-20 Thread Eric Liu via cfe-commits 
ioeric added a subscriber: ioeric.


Comment at: include-fixer/tool/ClangIncludeFixer.cpp:365
@@ +364,3 @@
+  for (const auto  : Context.getQuerySymbolInfos()) {
+Replacements->insert({FilePath, Info.Range.getOffset(),
+  Info.Range.getLength(),

Now that we insert namespace qualifiers, we might want to do a 
`formatReplacements` before applying them.


https://reviews.llvm.org/D22567



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