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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
