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

[Haojian Wu via cfe-commits]

hokein added inline comments.

================
Comment at: include-fixer/IncludeFixer.cpp:258
@@ +257,3 @@
+
+    // Query the symbol based on C++ name Lookup rules.
+    // Firstly, lookup the identifier with scoped namespace contexts; If fails,
----------------
djasper wrote:
> Could you add something about this to the CL description? Or actually, I think this is very separate from fixing the namespace qualifiers. Can we move this part to a different patch?
> 
> Or am I missing something so that one cannot go without the other? Fundamentally, this seems to be doing two things:
> 1) Insert namespace qualifiers if they are missing.
> 2) Take existing namespace qualifiers to ensure we aren't looking up the wrong symbol.
It's not introduced by this patch. Actually it moves from `query` function caller to the function body. Why we make this change? Because we need to save the scoped qualifiers of the symbol, so that include-fixer can know how to create a **correct** qualifiers for the symbol.

================
Comment at: include-fixer/IncludeFixerContext.h:31
@@ +30,3 @@
+        MatchedSymbols(Symbols), SymbolRange(Range) {
+    // Deduplicate headers, so that we don't want to suggest the same header
+    // twice.
----------------
djasper wrote:
> This again seems like something that is unrelated to what the patch is actually meant to do. Can we split this into a separate patch?
This is also a move from `rankByPopularity` in `SymbolIndexManager.cpp`.

================
Comment at: unittests/include-fixer/IncludeFixerTest.cpp:144
@@ -141,1 +143,3 @@
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/false, IncludePath));
 
----------------
djasper wrote:
> Do you need to set FixNamespaceQualifiers to false here?
No need to. Both are OK. Change to `true` here.


http://reviews.llvm.org/D21603