Index: clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h +++ clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h @@ -26,10 +26,13 @@ const std::vector Symbols, tooling::Range Range); - /// \brief Create a replacement for adding missing namespace qualifiers to the - /// symbol. - tooling::Replacement createSymbolReplacement(llvm::StringRef FilePath, - size_t Idx = 0); + struct HeaderInfo { + /// \brief The header where QualifiedName comes from. + std::string Header; + /// \brief A symbol name with completed namespace qualifiers which will + /// replace the original symbol. + std::string QualifiedName; + }; /// \brief Get symbol name. llvm::StringRef getSymbolIdentifier() const { return SymbolIdentifier; } @@ -37,19 +40,13 @@ /// \brief Get replacement range of the symbol. tooling::Range getSymbolRange() const { return SymbolRange; } - /// \brief Get all matched symbols. - const std::vector &getMatchedSymbols() const { - return MatchedSymbols; - } - - /// \brief Get all headers. The headers are sorted in a descending order based - /// on the popularity info in SymbolInfo. - const std::vector &getHeaders() const { return Headers; } + const std::vector &getHeaderInfos() const { return HeaderInfos; } private: friend struct llvm::yaml::MappingTraits; - /// \brief The symbol name. + /// \brief The raw symbol name being queried in database. This name might miss + /// some namespace qualifiers, and will be replaced by a fully qualified one. std::string SymbolIdentifier; /// \brief The qualifiers of the scope in which SymbolIdentifier lookup @@ -58,15 +55,15 @@ /// Empty if SymbolIdentifier is not in a specific scope. std::string SymbolScopedQualifiers; - /// \brief The headers which have SymbolIdentifier definitions. - std::vector Headers; - /// \brief The symbol candidates which match SymbolIdentifier. The symbols are /// sorted in a descending order based on the popularity info in SymbolInfo. std::vector MatchedSymbols; /// \brief The replacement range of SymbolIdentifier. tooling::Range SymbolRange; + + /// \brief The header information. + std::vector HeaderInfos; }; } // namespace include_fixer Index: clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp +++ clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp @@ -13,32 +13,19 @@ namespace clang { namespace include_fixer { -IncludeFixerContext::IncludeFixerContext( - llvm::StringRef Name, llvm::StringRef ScopeQualifiers, - const std::vector Symbols, - tooling::Range Range) - : SymbolIdentifier(Name), SymbolScopedQualifiers(ScopeQualifiers), - MatchedSymbols(Symbols), SymbolRange(Range) { - // Deduplicate headers, so that we don't want to suggest the same header - // twice. - for (const auto &Symbol : MatchedSymbols) - Headers.push_back(Symbol.getFilePath()); - Headers.erase(std::unique(Headers.begin(), Headers.end(), - [](const std::string &A, const std::string &B) { - return A == B; - }), - Headers.end()); -} +namespace { -tooling::Replacement -IncludeFixerContext::createSymbolReplacement(llvm::StringRef FilePath, - size_t Idx) { - assert(Idx < MatchedSymbols.size()); +std::string createQualifiedNameForReplacement( + llvm::StringRef RawSymbolName, + llvm::StringRef SymbolScopedQualifiers, + const find_all_symbols::SymbolInfo &MatchedSymbol) { // No need to add missing qualifiers if SymbolIndentifer has a global scope // operator "::". - if (getSymbolIdentifier().startswith("::")) - return tooling::Replacement(); - std::string QualifiedName = MatchedSymbols[Idx].getQualifiedName(); + if (RawSymbolName.startswith("::")) + return RawSymbolName; + + std::string QualifiedName = MatchedSymbol.getQualifiedName(); + // For nested classes, the qualified name constructed from database misses // some stripped qualifiers, because when we search a symbol in database, // we strip qualifiers from the end until we find a result. So append the @@ -46,7 +33,7 @@ // // Get stripped qualifiers. llvm::SmallVector SymbolQualifiers; - getSymbolIdentifier().split(SymbolQualifiers, "::"); + RawSymbolName.split(SymbolQualifiers, "::"); std::string StrippedQualifiers; while (!SymbolQualifiers.empty() && !llvm::StringRef(QualifiedName).endswith(SymbolQualifiers.back())) { @@ -56,9 +43,30 @@ // Append the missing stripped qualifiers. std::string FullyQualifiedName = QualifiedName + StrippedQualifiers; auto pos = FullyQualifiedName.find(SymbolScopedQualifiers); - return {FilePath, SymbolRange.getOffset(), SymbolRange.getLength(), - FullyQualifiedName.substr( - pos == std::string::npos ? 0 : SymbolScopedQualifiers.size())}; + return FullyQualifiedName.substr( + pos == std::string::npos ? 0 : SymbolScopedQualifiers.size()); +} + +} // anonymous namespace + +IncludeFixerContext::IncludeFixerContext( + llvm::StringRef Name, llvm::StringRef ScopeQualifiers, + std::vector Symbols, + tooling::Range Range) + : SymbolIdentifier(Name), SymbolScopedQualifiers(ScopeQualifiers), + MatchedSymbols(std::move(Symbols)), SymbolRange(Range) { + for (const auto &Symbol : MatchedSymbols) { + HeaderInfos.push_back({Symbol.getFilePath().str(), + createQualifiedNameForReplacement( + SymbolIdentifier, ScopeQualifiers, Symbol)}); + } + // Deduplicate header infos. + HeaderInfos.erase(std::unique(HeaderInfos.begin(), HeaderInfos.end(), + [](const HeaderInfo &A, const HeaderInfo &B) { + return A.Header == B.Header && + A.QualifiedName == B.QualifiedName; + }), + HeaderInfos.end()); } } // include_fixer Index: clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp +++ clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp @@ -27,13 +27,44 @@ LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(IncludeFixerContext) LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string) +LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::HeaderInfo) namespace llvm { namespace yaml { + +template <> struct MappingTraits { + struct NormalizedRange { + NormalizedRange(const IO &) : Offset(0), Length(0) {} + + NormalizedRange(const IO &, const tooling::Range &R) + : Offset(R.getOffset()), Length(R.getLength()) {} + + tooling::Range denormalize(const IO &) { + return tooling::Range(Offset, Length); + } + + unsigned Offset; + unsigned Length; + }; + static void mapping(IO &IO, tooling::Range &Info) { + MappingNormalization Keys(IO, Info); + IO.mapRequired("Offset", Keys->Offset); + IO.mapRequired("Length", Keys->Length); + } +}; + +template <> struct MappingTraits { + static void mapping(IO &io, IncludeFixerContext::HeaderInfo &Info) { + io.mapRequired("Header", Info.Header); + io.mapRequired("QualifiedName", Info.QualifiedName); + } +}; + template <> struct MappingTraits { - static void mapping(IO &io, IncludeFixerContext &Context) { - io.mapRequired("SymbolIdentifier", Context.SymbolIdentifier); - io.mapRequired("Headers", Context.Headers); + static void mapping(IO &IO, IncludeFixerContext &Context) { + IO.mapRequired("SymbolIdentifier", Context.SymbolIdentifier); + IO.mapRequired("HeaderInfos", Context.HeaderInfos); + IO.mapRequired("Range", Context.SymbolRange); } }; } // namespace yaml @@ -81,7 +112,9 @@ "JSON format to stdout:\n" " {\n" " \"SymbolIdentifier\": \"foo\",\n" - " \"Headers\": [\"\\\"foo_a.h\\\"\"]\n" + " \"Range\": {\"Offset\":0, \"Length\": 3},\n" + " \"HeaderInfos\": [ {\"Header\": \"\\\"foo_a.h\\\"\",\n" + " \"QualifiedName\": \"a::foo\"} ]\n" " }"), cl::init(false), cl::cat(IncludeFixerCategory)); @@ -90,8 +123,11 @@ cl::desc("Insert a specific header. This should run with STDIN mode.\n" "The result is written to stdout. It is currently used for\n" "editor integration. Support YAML/JSON format:\n" - " -insert-header=\"{SymbolIdentifier: foo,\n" - " Headers: ['\\\"foo_a.h\\\"']}\""), + " -insert-header=\"{\n" + " SymbolIdentifier: foo,\n" + " Range: {Offset: 0, Length: 3},\n" + " HeaderInfos: [ {Headers: \"\\\"foo_a.h\\\"\",\n" + " QualifiedName: \"a::foo\"} ]}\""), cl::init(""), cl::cat(IncludeFixerCategory)); cl::opt @@ -155,15 +191,22 @@ void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) { OS << "{\n" - " \"SymbolIdentifier\": \"" << Context.getSymbolIdentifier() << "\",\n" - " \"Headers\": [ "; - for (const auto &Header : Context.getHeaders()) { - OS << " \"" << llvm::yaml::escape(Header) << "\""; - if (Header != Context.getHeaders().back()) - OS << ", "; - } - OS << " ]\n" - "}\n"; + " \"SymbolIdentifier\": \"" + << Context.getSymbolIdentifier() << "\",\n"; + OS << " \"Range\": {"; + OS << " \"Offset\":" << Context.getSymbolRange().getOffset() << ","; + OS << " \"Length\":" << Context.getSymbolRange().getLength() << " },\n"; + OS << " \"HeaderInfos\": [\n"; + const auto &HeaderInfos = Context.getHeaderInfos(); + for (const auto &Info : HeaderInfos) { + OS << " {\"Header\": \"" << llvm::yaml::escape(Info.Header) << "\",\n" + << " \"QualifiedName\": \"" << Info.QualifiedName << "\"}"; + if (&Info != &HeaderInfos.back()) + OS << ",\n"; + } + OS << "\n"; + OS << " ]\n"; + OS << "}\n"; } int includeFixerMain(int argc, const char **argv) { @@ -204,18 +247,44 @@ IncludeFixerContext Context; yin >> Context; - if (Context.getHeaders().size() != 1) { - errs() << "Expect exactly one inserted header.\n"; + const auto &HeaderInfos = Context.getHeaderInfos(); + assert(!HeaderInfos.empty()); + // We only accept one unique header. + // Check all elements in HeaderInfos have the same header. + bool IsUniqueHeader = std::equal( + HeaderInfos.begin()+1, HeaderInfos.end(), HeaderInfos.begin(), + [](const IncludeFixerContext::HeaderInfo &LHS, + const IncludeFixerContext::HeaderInfo &RHS) { + return LHS.Header == RHS.Header; + }); + if (!IsUniqueHeader) { + errs() << "Expect exactly one unique header.\n"; return 1; } auto Replacements = clang::include_fixer::createInsertHeaderReplacements( - Code->getBuffer(), FilePath, Context.getHeaders().front(), InsertStyle); + Code->getBuffer(), FilePath, Context.getHeaderInfos().front().Header, + InsertStyle); if (!Replacements) { errs() << "Failed to create header insertion replacement: " << llvm::toString(Replacements.takeError()) << "\n"; return 1; } + + // If a header have multiple symbols, we won't add the missing namespace + // qualifiers because we don't know which one is exactly used. + // + // Check whether all elements in HeaderInfos have the same qualified name. + bool IsUniqueQualifiedName = std::equal( + HeaderInfos.begin() + 1, HeaderInfos.end(), HeaderInfos.begin(), + [](const IncludeFixerContext::HeaderInfo &LHS, + const IncludeFixerContext::HeaderInfo &RHS) { + return LHS.QualifiedName == RHS.QualifiedName; + }); + if (IsUniqueQualifiedName) + Replacements->insert({FilePath, Context.getSymbolRange().getOffset(), + Context.getSymbolRange().getLength(), + Context.getHeaderInfos().front().QualifiedName}); auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), *Replacements); if (!ChangedCode) { @@ -248,7 +317,7 @@ return 0; } - if (Context.getMatchedSymbols().empty()) + if (Context.getHeaderInfos().empty()) return 0; auto Buffer = llvm::MemoryBuffer::getFile(FilePath); @@ -257,10 +326,9 @@ return 1; } - // FIXME: Rank the results and pick the best one instead of the first one. auto Replacements = clang::include_fixer::createInsertHeaderReplacements( /*Code=*/Buffer.get()->getBuffer(), FilePath, - Context.getHeaders().front(), InsertStyle); + Context.getHeaderInfos().front().Header, InsertStyle); if (!Replacements) { errs() << "Failed to create header insertion replacement: " << llvm::toString(Replacements.takeError()) << "\n"; @@ -268,11 +336,12 @@ } if (!Quiet) - llvm::errs() << "Added #include" << Context.getHeaders().front(); + llvm::errs() << "Added #include" << Context.getHeaderInfos().front().Header; // Add missing namespace qualifiers to the unidentified symbol. - if (Context.getSymbolRange().getLength() > 0) - Replacements->insert(Context.createSymbolReplacement(FilePath, 0)); + Replacements->insert({FilePath, Context.getSymbolRange().getOffset(), + Context.getSymbolRange().getLength(), + Context.getHeaderInfos().front().QualifiedName}); // Set up a new source manager for applying the resulting replacements. IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); Index: clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py +++ clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py @@ -115,30 +115,44 @@ include_fixer_context = json.loads(stdout) symbol = include_fixer_context["SymbolIdentifier"] - headers = include_fixer_context["Headers"] + # The header_infos is already sorted by include-fixer. + header_infos = include_fixer_context["HeaderInfos"] + # Deduplicate headers while keeping the order, so that the same header would + # not be suggested twice. + unique_headers = [] + seen = set() + for header_info in header_infos: + header = header_info["Header"] + if header not in seen: + seen.add(header) + unique_headers.append(header) if not symbol: print "The file is fine, no need to add a header.\n" return - if not headers: + if not unique_headers: print "Couldn't find a header for {0}.\n".format(symbol) return - # The first line is the symbol name. # If there is only one suggested header, insert it directly. - if len(headers) == 1 or maximum_suggested_headers == 1: + if len(unique_headers) == 1 or maximum_suggested_headers == 1: InsertHeaderToVimBuffer({"SymbolIdentifier": symbol, - "Headers": [headers[0]]}, text) - print "Added #include {0} for {1}.\n".format(headers[0], symbol) + "Range": include_fixer_context["Range"], + "HeaderInfos": header_infos}, text) + print "Added #include {0} for {1}.\n".format(unique_headers[0], symbol) return try: selected = GetUserSelection("choose a header file for {0}.".format(symbol), - headers, maximum_suggested_headers) + unique_headers, maximum_suggested_headers) + selected_header_infos = [ + header for header in header_infos if header["Header"] == selected] + # Insert a selected header. InsertHeaderToVimBuffer({"SymbolIdentifier": symbol, - "Headers": [selected]}, text) + "Range": include_fixer_context["Range"], + "HeaderInfos": selected_header_infos}, text) print "Added #include {0} for {1}.\n".format(selected, symbol) except Exception as error: print >> sys.stderr, error.message Index: clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp =================================================================== --- clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp +++ clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp @@ -1,11 +1,16 @@ // REQUIRES: shell -// RUN: sed -e 's#//.*$##' %s > %t.cpp -// RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s -check-prefix=CHECK-HEADERS -// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Headers: ["\"foo.h\""]}' %t.cpp | FileCheck %s -check-prefix=CHECK +// RUN: echo "foo f;" > %t.cpp +// RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s +// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Range: {Offset: 0, Length: 3}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE +// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Range: {Offset: 0, Length: 3}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp +// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Range: {Offset: 0, Length: 3}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp // -// CHECK-HEADERS: "Headers": [ "\"foo.h\"", "\"bar.h\"" ] +// CHECK: "HeaderInfos": [ +// CHECK-NEXT: {"Header": "\"foo.h\"", +// CHECK-NEXT: "QualifiedName": "foo"}, +// CHECK-NEXT: {"Header": "\"bar.h\"", +// CHECK-NEXT: "QualifiedName": "foo"} +// CHECK-NEXT:] // -// CHECK: #include "foo.h" -// CHECK: foo f; - -foo f; +// CHECK-CODE: #include "foo.h" +// CHECK-CODE: foo f; Index: clang-tools-extra/trunk/test/include-fixer/ranking.cpp =================================================================== --- clang-tools-extra/trunk/test/include-fixer/ranking.cpp +++ clang-tools-extra/trunk/test/include-fixer/ranking.cpp @@ -1,5 +1,10 @@ // RUN: clang-include-fixer -db=yaml -input=%S/Inputs/fake_yaml_db.yaml -output-headers %s -- | FileCheck %s -// CHECK: "Headers": [ "\"../include/bar.h\"", "\"../include/zbar.h\"" ] +// CHECK: "HeaderInfos": [ +// CHECK-NEXT: {"Header": "\"../include/bar.h\"", +// CHECK-NEXT: "QualifiedName": "b::a::bar"}, +// CHECK-NEXT: {"Header": "\"../include/zbar.h\"", +// CHECK-NEXT: "QualifiedName": "b::a::bar"} +// CHECK-NEXT:] bar b; Index: clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp =================================================================== --- clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp +++ clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp @@ -84,18 +84,19 @@ std::string FakeFileName = "input.cc"; runOnCode(&Factory, Code, FakeFileName, ExtraArgs); - if (FixerContext.getMatchedSymbols().empty()) + if (FixerContext.getHeaderInfos().empty()) return Code; auto Replaces = clang::include_fixer::createInsertHeaderReplacements( - Code, FakeFileName, FixerContext.getHeaders().front()); + Code, FakeFileName, FixerContext.getHeaderInfos().front().Header); EXPECT_TRUE(static_cast(Replaces)) << llvm::toString(Replaces.takeError()) << "\n"; if (!Replaces) return ""; clang::RewriterTestContext Context; clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code); - if (FixerContext.getSymbolRange().getLength() > 0) - Replaces->insert(FixerContext.createSymbolReplacement(FakeFileName, 0)); + Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(), + FixerContext.getSymbolRange().getLength(), + FixerContext.getHeaderInfos().front().QualifiedName}); clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite); return Context.getRewrittenText(ID); }