[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<bool> 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