Re: [PATCH] D21603: [include-fixer] Fix namespace after inserting a missing header.

[Daniel Jasper via cfe-commits]

djasper added inline comments.


Comment at: include-fixer/IncludeFixer.cpp:234
@@ +233,3 @@
+  std::string MinimizedFilePath = minimizeInclude(
+  ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
+  : "\"" + FilePath + 
"\""),

How is this change related?


Comment at: include-fixer/IncludeFixer.cpp:238
@@ +237,3 @@
+  SymbolCandidates.emplace_back(Symbol.getName(), Symbol.getSymbolKind(),
+   MinimizedFilePath, Symbol.getLineNumber(),
+   Symbol.getContexts(),

Indentation is off.


Comment at: include-fixer/tool/ClangIncludeFixer.cpp:88
@@ -86,1 +87,3 @@
 
+cl::opt FixNamespace("fix-namespace",
+   cl::desc("Add missing namespace prefix to the "

Do we really want this? Should we just always do it? Who is going to use this 
flag?

If we want it, maybe call it "fix-namespace-qualifiers"? This renaming might 
also make sense of the patch itself, the test case, ...


Comment at: unittests/include-fixer/IncludeFixerTest.cpp:235
@@ -223,1 +234,3 @@
 
+TEST(IncludeFixer, FixNamespace) {
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",

All the tests in here test with an existing NestedNameSpecifier (i.e. b::bar). 
Is there a reason to not also have tests with a plain identifier (e.g. bar)?


Comment at: unittests/include-fixer/IncludeFixerTest.cpp:253
@@ +252,3 @@
+
+  // FIXME: Fix-namespace should not add the missing namespace prefix to the
+  // unidentified symbol which is already in that namespace.

I think we should address this part from the start. Otherwise, we are making 
the current behavior worse for a significant amount of cases. I suspect that 
frequently people use types from their own projects (and thus in the same 
namespace they are already in) and we don't want to add nested name specifiers 
for those.


http://reviews.llvm.org/D21603



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

Re: [PATCH] D21603: [include-fixer] Fix namespace after inserting a missing header.

[Haojian Wu via cfe-commits]

hokein added a comment.

In http://reviews.llvm.org/D21603#468717, @djasper wrote:

> Sorry, I completely forgot about this. Will try to review today. Is this part 
> about the patch description accurate?


Yes, the description is accurate.

> Specifically, what needs to be implemented in vim to make this work?


We need a way to dump the mapping relationship between symbols and their 
headers, so that include-fixer can know which namespace prefix should be used 
when user selects a particular header in vim.


http://reviews.llvm.org/D21603



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

Re: [PATCH] D21603: [include-fixer] Fix namespace after inserting a missing header.

[Daniel Jasper via cfe-commits]

djasper added a comment.

Sorry, I completely forgot about this. Will try to review today. Is this part 
about the patch description accurate?

"This version only fixes the first discovered unidentified symbol.
In the long run, include-fixer should fix all unidentified symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet."

Specifically, what needs to be implemented in vim to make this work?


http://reviews.llvm.org/D21603



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

Re: [PATCH] D21603: [include-fixer] Fix namespace after inserting a missing header.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 61524.
hokein added a comment.

Fix a typo.


http://reviews.llvm.org/D21603

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/IncludeFixerContext.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/SymbolIndexManager.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/tool/ClangIncludeFixer.cpp
  unittests/include-fixer/IncludeFixerTest.cpp

Index: unittests/include-fixer/IncludeFixerTest.cpp
===
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -50,7 +50,7 @@
 }
 
 static std::string runIncludeFixer(
-StringRef Code,
+StringRef Code, bool FixNamespace = false,
 const std::vector  = std::vector()) {
   std::vector Symbols = {
   SymbolInfo("string", SymbolInfo::SymbolKind::Class, "", 1,
@@ -82,14 +82,21 @@
   IncludeFixerContext FixerContext;
   IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
 
-  runOnCode(, Code, "input.cc", ExtraArgs);
-  if (FixerContext.Headers.empty())
+  std::string FakeFileName = "input.cc";
+  runOnCode(, Code, FakeFileName, ExtraArgs);
+  if (FixerContext.getMatchedSymbols().empty())
 return Code;
   tooling::Replacements Replacements =
   clang::include_fixer::createInsertHeaderReplacements(
-  Code, "input.cc", FixerContext.Headers.front());
+  Code, FakeFileName, FixerContext.getHeaders().front());
   clang::RewriterTestContext Context;
-  clang::FileID ID = Context.createInMemoryFile("input.cc", Code);
+  clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
+  if (FixNamespace && FixerContext.getSymbolRange().getLength() > 0) {
+Replacements.emplace(
+FakeFileName, FixerContext.getSymbolRange().getOffset(),
+FixerContext.getSymbolRange().getLength(),
+FixerContext.getMatchedSymbols().front().getFullyQualifiedName());
+  }
   clang::tooling::applyAllReplacements(Replacements, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
@@ -136,20 +143,24 @@
 
 TEST(IncludeFixer, MinimizeInclude) {
   std::vector IncludePath = {"-Idir/"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-isystemdir"};
-  EXPECT_EQ("#include \na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-iquotedir"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-Idir", "-Idir/otherdir"};
-  EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 }
 
 TEST(IncludeFixer, NestedName) {
@@ -221,6 +232,31 @@
 runIncludeFixer("a::Vector v;"));
 }
 
+TEST(IncludeFixer, FixNamespace) {
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("a::b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ(
+  "c::b::bar b;\n",
+  runIncludeFixer("c::b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n",
+runIncludeFixer("int test = Green;\n", /*FixNamespace=*/true));
+
+  // FIXME: Fix-namespace should not fix the global qualified identifier.
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("::a::b::bar b;\n", /*FixNamespace=*/true));
+
+  // FIXME: Fix-namespace should not add the missing namespace prefix to the
+  // unidentified symbol which is already in that namespace.
+  EXPECT_EQ(
+  "#include \"bar.h\"\nnamespace a {\na::b::bar b;\n}\n",
+  runIncludeFixer("namespace a {\nb::bar b;\n}\n", /*FixNamespace==*/true));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang
Index: include-fixer/tool/ClangIncludeFixer.cpp
===
--- include-fixer/tool/ClangIncludeFixer.cpp
+++ include-fixer/tool/ClangIncludeFixer.cpp
@@ -16,6 +16,7 @@
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"

[PATCH] D21603: [include-fixer] Fix namespace after inserting a missing header.

[Haojian Wu via cfe-commits]

hokein created this revision.
hokein added a subscriber: cfe-commits.

This is an initial version of fixing namespace issues by adding a missing
namespace prefix to an unidentified symbol.

This version only fixes the first discovered unidentified symbol
In the long run, include-fixer should fix all unidentfied symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet.

http://reviews.llvm.org/D21603

Files:
  include-fixer/IncludeFixer.cpp
  include-fixer/IncludeFixerContext.h
  include-fixer/SymbolIndexManager.cpp
  include-fixer/SymbolIndexManager.h
  include-fixer/find-all-symbols/SymbolInfo.cpp
  include-fixer/find-all-symbols/SymbolInfo.h
  include-fixer/tool/ClangIncludeFixer.cpp
  unittests/include-fixer/IncludeFixerTest.cpp

Index: unittests/include-fixer/IncludeFixerTest.cpp
===
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -50,7 +50,7 @@
 }
 
 static std::string runIncludeFixer(
-StringRef Code,
+StringRef Code, bool FixNamespace = false,
 const std::vector  = std::vector()) {
   std::vector Symbols = {
   SymbolInfo("string", SymbolInfo::SymbolKind::Class, "", 1,
@@ -82,14 +82,21 @@
   IncludeFixerContext FixerContext;
   IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
 
-  runOnCode(, Code, "input.cc", ExtraArgs);
-  if (FixerContext.Headers.empty())
+  std::string FakeFileName = "input.cc";
+  runOnCode(, Code, FakeFileName, ExtraArgs);
+  if (FixerContext.getMatchedSymbols().empty())
 return Code;
   tooling::Replacements Replacements =
   clang::include_fixer::createInsertHeaderReplacements(
-  Code, "input.cc", FixerContext.Headers.front());
+  Code, FakeFileName, FixerContext.getHeaders().front());
   clang::RewriterTestContext Context;
-  clang::FileID ID = Context.createInMemoryFile("input.cc", Code);
+  clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
+  if (FixNamespace && FixerContext.getSymbolRange().getLength() > 0) {
+Replacements.emplace(
+FakeFileName, FixerContext.getSymbolRange().getOffset(),
+FixerContext.getSymbolRange().getLength(),
+FixerContext.getMatchedSymbols().front().getFullyQualifiedName());
+  }
   clang::tooling::applyAllReplacements(Replacements, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
@@ -136,20 +143,24 @@
 
 TEST(IncludeFixer, MinimizeInclude) {
   std::vector IncludePath = {"-Idir/"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-isystemdir"};
-  EXPECT_EQ("#include \na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-iquotedir"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 
   IncludePath = {"-Idir", "-Idir/otherdir"};
-  EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n",
-runIncludeFixer("a::b::foo bar;\n", IncludePath));
+  EXPECT_EQ(
+  "#include \"qux.h\"\na::b::foo bar;\n",
+  runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
 }
 
 TEST(IncludeFixer, NestedName) {
@@ -221,6 +232,31 @@
 runIncludeFixer("a::Vector v;"));
 }
 
+TEST(IncludeFixer, FixNamespace) {
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("a::b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ(
+  "c::b::bar b;\n",
+  runIncludeFixer("c::b::bar b;\n", /*FixNamespace=*/true));
+
+  EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n",
+runIncludeFixer("int test = Green;\n", /*FixNamespace=*/true));
+
+  // FIXME: Fix-namespace should not fix the global qualified identifier.
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+runIncludeFixer("::a::b::bar b;\n", /*FixNamespace=*/true));
+
+  // FIXME: Fix-namespace should not add the missing namespace prefix to the
+  // unidentified symbol which is already in that namespace.
+  EXPECT_EQ(
+  "#include \"bar.h\"\nnamespace a {\na::b::bar b;\n}\n",
+  runIncludeFixer("namespace a {\nb::bar b;\n}\n", /*FixNamespace==*/true));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang
Index: