djasper added inline comments. ================ Comment at: include-fixer/IncludeFixer.cpp:224 @@ +223,3 @@ + std::string MinimizedFilePath = minimizeInclude( + ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath + : "\"" + FilePath + "\""), ---------------- Ah, I see. This was just moved. Sorry, missed that.
================ 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, ---------------- 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. ================ 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. ---------------- 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? ================ Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:93 @@ -92,1 +92,3 @@ +std::string SymbolInfo::getFullyQualifiedName() const { + std::string FullyQuanlifiedName = Name; ---------------- I think "getQualifiedName" is sufficient and then matches what's in NamedDecl. ================ Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:94 @@ +93,3 @@ +std::string SymbolInfo::getFullyQualifiedName() const { + std::string FullyQuanlifiedName = Name; + for (const auto &Context : Contexts) { ---------------- There is an extra "n" in "FullyQuanlifiedName". ================ Comment at: include-fixer/tool/ClangIncludeFixer.cpp:88 @@ -86,1 +87,3 @@ +cl::opt<bool> FixNamespaceQualifiers("fix-namespace-qualifiers", + cl::desc("Add missing namespace qualifiers to the " ---------------- I'd strongly recommend not doing that. Removing flags that people are using is hard (breaks them). Make this good enough so that it is always the right thing to do. ================ Comment at: unittests/include-fixer/IncludeFixerTest.cpp:144 @@ -141,1 +143,3 @@ + runIncludeFixer("a::b::foo bar;\n", + /*FixNamespaceQualifiers=*/false, IncludePath)); ---------------- Do you need to set FixNamespaceQualifiers to false 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