hokein created this revision.
hokein added a reviewer: bkramer.
hokein added a subscriber: cfe-commits.
https://reviews.llvm.org/D22567
Files:
include-fixer/IncludeFixer.cpp
include-fixer/IncludeFixerContext.cpp
include-fixer/IncludeFixerContext.h
include-fixer/tool/ClangIncludeFixer.cpp
include-fixer/tool/clang-include-fixer.py
test/include-fixer/commandline_options.cpp
unittests/include-fixer/IncludeFixerTest.cpp
Index: unittests/include-fixer/IncludeFixerTest.cpp
===================================================================
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -97,9 +97,11 @@
return "";
clang::RewriterTestContext Context;
clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
- Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(),
- FixerContext.getSymbolRange().getLength(),
- FixerContext.getHeaderInfos().front().QualifiedName});
+ for (const auto &Info : FixerContext.getQuerySymbolInfos()) {
+ Replaces->insert({FakeFileName, Info.Range.getOffset(),
+ Info.Range.getLength(),
+ FixerContext.getHeaderInfos().front().QualifiedName});
+ }
clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite);
return Context.getRewrittenText(ID);
}
@@ -275,6 +277,67 @@
runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n"));
}
+TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) {
+ const char TestCode[] = R"(
+namespace a {
+bar b; // Fix
+int func1() {
+ bar a; // Fix
+ bar *p = new bar(); // Fix
+ return 0;
+}
+} // namespace a
+
+namespace a {
+bar func2() { // Fix
+ bar f; // Fix
+ return f;
+}
+} // namespace a
+
+void f() {
+ bar b; // No-fix: Not in the same scope.
+}
+
+namespace a {
+namespace c {
+ bar b; // No-fix: Not in the same scope.
+} // namespace c
+} // namespace a
+)";
+
+ const char ExpectedCode[] = R"(
+#include "bar.h"
+namespace a {
+b::bar b; // Fix
+int func1() {
+ b::bar a; // Fix
+ b::bar *p = new b::bar(); // Fix
+ return 0;
+}
+} // namespace a
+
+namespace a {
+b::bar func2() { // Fix
+ b::bar f; // Fix
+ return f;
+}
+} // namespace a
+
+void f() {
+ bar b; // No-fix: Not in the same scope.
+}
+
+namespace a {
+namespace c {
+ bar b; // No-fix: Not in the same scope.
+} // namespace c
+} // namespace a
+)";
+
+ EXPECT_EQ(ExpectedCode, runIncludeFixer(TestCode));
+}
+
} // namespace
} // namespace include_fixer
} // namespace clang
Index: test/include-fixer/commandline_options.cpp
===================================================================
--- test/include-fixer/commandline_options.cpp
+++ test/include-fixer/commandline_options.cpp
@@ -1,9 +1,9 @@
// REQUIRES: shell
// 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='{QuerySymbolInfo: {RawIdentifier: 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='{QuerySymbolInfo: {RawIdentifier: 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='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: 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='{QuerySymbolInfos: [{RawIdentifier: 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='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
//
// CHECK: "HeaderInfos": [
// CHECK-NEXT: {"Header": "\"foo.h\"",
Index: include-fixer/tool/clang-include-fixer.py
===================================================================
--- include-fixer/tool/clang-include-fixer.py
+++ include-fixer/tool/clang-include-fixer.py
@@ -127,8 +127,8 @@
return
include_fixer_context = json.loads(stdout)
- query_symbol_info = include_fixer_context["QuerySymbolInfo"]
- symbol = query_symbol_info["RawIdentifier"]
+ query_symbol_infos = include_fixer_context["QuerySymbolInfos"]
+ symbol = query_symbol_infos[0]["RawIdentifier"]
# 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
@@ -152,7 +152,7 @@
try:
# If there is only one suggested header, insert it directly.
if len(unique_headers) == 1 or maximum_suggested_headers == 1:
- InsertHeaderToVimBuffer({"QuerySymbolInfo": query_symbol_info,
+ InsertHeaderToVimBuffer({"QuerySymbolInfos": query_symbol_infos,
"HeaderInfos": header_infos}, text)
print "Added #include {0} for {1}.".format(unique_headers[0], symbol)
return
@@ -163,7 +163,7 @@
header for header in header_infos if header["Header"] == selected]
# Insert a selected header.
- InsertHeaderToVimBuffer({"QuerySymbolInfo": query_symbol_info,
+ InsertHeaderToVimBuffer({"QuerySymbolInfos": query_symbol_infos,
"HeaderInfos": selected_header_infos}, text)
print "Added #include {0} for {1}.".format(selected, symbol)
except Exception as error:
Index: include-fixer/tool/ClangIncludeFixer.cpp
===================================================================
--- include-fixer/tool/ClangIncludeFixer.cpp
+++ include-fixer/tool/ClangIncludeFixer.cpp
@@ -29,6 +29,7 @@
LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(IncludeFixerContext)
LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string)
LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::HeaderInfo)
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::QuerySymbolInfo)
namespace llvm {
namespace yaml {
@@ -70,7 +71,7 @@
template <> struct MappingTraits<IncludeFixerContext> {
static void mapping(IO &IO, IncludeFixerContext &Context) {
- IO.mapRequired("QuerySymbolInfo", Context.QuerySymbol);
+ IO.mapRequired("QuerySymbolInfos", Context.QuerySymbolInfos);
IO.mapRequired("HeaderInfos", Context.HeaderInfos);
}
};
@@ -118,10 +119,10 @@
cl::desc("Print the symbol being queried and all its relevant headers in\n"
"JSON format to stdout:\n"
" {\n"
- " \"QuerySymbolInfo\": {\n"
- " \"RawIdentifier\": \"foo\",\n"
- " \"Range\": {\"Offset\": 0, \"Length\": 3}\n"
- " },\n"
+ " \"QuerySymbolInfos\": [\n"
+ " {\"RawIdentifier\": \"foo\",\n"
+ " \"Range\": {\"Offset\": 0, \"Length\": 3}}\n"
+ " ],\n"
" \"HeaderInfos\": [ {\"Header\": \"\\\"foo_a.h\\\"\",\n"
" \"QualifiedName\": \"a::foo\"} ]\n"
" }"),
@@ -133,10 +134,10 @@
"The result is written to stdout. It is currently used for\n"
"editor integration. Support YAML/JSON format:\n"
" -insert-header=\"{\n"
- " QuerySymbolInfo: {\n"
- " RawIdentifier: foo,\n"
- " Range: {Offset: 0, Length: 3}\n"
- " },\n"
+ " QuerySymbolInfos: [\n"
+ " {RawIdentifier: foo,\n"
+ " Range: {Offset: 0, Length: 3}}\n"
+ " ],\n"
" HeaderInfos: [ {Headers: \"\\\"foo_a.h\\\"\",\n"
" QualifiedName: \"a::foo\"} ]}\""),
cl::init(""), cl::cat(IncludeFixerCategory));
@@ -202,13 +203,16 @@
void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) {
OS << "{\n"
- " \"QuerySymbolInfo\": {\n"
- " \"RawIdentifier\": \""
- << Context.getSymbolIdentifier() << "\",\n";
- OS << " \"Range\": {";
- OS << " \"Offset\":" << Context.getSymbolRange().getOffset() << ",";
- OS << " \"Length\":" << Context.getSymbolRange().getLength() << " }\n";
- OS << " },\n";
+ " \"QuerySymbolInfos\": [\n";
+ for (const auto &Info : Context.getQuerySymbolInfos()) {
+ OS << " {\"RawIdentifier\": \"" << Info.RawIdentifier << "\",\n";
+ OS << " \"Range\":{";
+ OS << "\"Offset\":" << Info.Range.getOffset() << ",";
+ OS << "\"Length\":" << Info.Range.getLength() << "}}";
+ if (&Info != &Context.getQuerySymbolInfos().back())
+ OS << ",\n";
+ }
+ OS << "\n ],\n";
OS << " \"HeaderInfos\": [\n";
const auto &HeaderInfos = Context.getHeaderInfos();
for (const auto &Info : HeaderInfos) {
@@ -294,10 +298,13 @@
const IncludeFixerContext::HeaderInfo &RHS) {
return LHS.QualifiedName == RHS.QualifiedName;
});
- if (IsUniqueQualifiedName)
- Replacements->insert({FilePath, Context.getSymbolRange().getOffset(),
- Context.getSymbolRange().getLength(),
- Context.getHeaderInfos().front().QualifiedName});
+ if (IsUniqueQualifiedName) {
+ for (const auto &Info : Context.getQuerySymbolInfos()) {
+ Replacements->insert({FilePath, Info.Range.getOffset(),
+ Info.Range.getLength(),
+ Context.getHeaderInfos().front().QualifiedName});
+ }
+ }
auto ChangedCode =
tooling::applyAllReplacements(Code->getBuffer(), *Replacements);
if (!ChangedCode) {
@@ -354,9 +361,11 @@
<< '\n';
// Add missing namespace qualifiers to the unidentified symbol.
- Replacements->insert({FilePath, Context.getSymbolRange().getOffset(),
- Context.getSymbolRange().getLength(),
- Context.getHeaderInfos().front().QualifiedName});
+ for (const auto &Info : Context.getQuerySymbolInfos()) {
+ Replacements->insert({FilePath, Info.Range.getOffset(),
+ Info.Range.getLength(),
+ Context.getHeaderInfos().front().QualifiedName});
+ }
// Set up a new source manager for applying the resulting replacements.
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
Index: include-fixer/IncludeFixerContext.h
===================================================================
--- include-fixer/IncludeFixerContext.h
+++ include-fixer/IncludeFixerContext.h
@@ -46,31 +46,39 @@
};
IncludeFixerContext() = default;
- IncludeFixerContext(const QuerySymbolInfo &QuerySymbol,
+ IncludeFixerContext(std::vector<QuerySymbolInfo> QuerySymbols,
std::vector<find_all_symbols::SymbolInfo> Symbols);
/// \brief Get symbol name.
llvm::StringRef getSymbolIdentifier() const {
- return QuerySymbol.RawIdentifier;
+ return QuerySymbolInfos.front().RawIdentifier;
}
/// \brief Get replacement range of the symbol.
- tooling::Range getSymbolRange() const { return QuerySymbol.Range; }
+ tooling::Range getSymbolRange() const {
+ return QuerySymbolInfos.front().Range;
+ }
+ /// \brief Get header information.
const std::vector<HeaderInfo> &getHeaderInfos() const { return HeaderInfos; }
+ /// \brief Get information of symbols being querid.
+ const std::vector<QuerySymbolInfo> &getQuerySymbolInfos() const {
+ return QuerySymbolInfos;
+ }
+
private:
friend struct llvm::yaml::MappingTraits<IncludeFixerContext>;
+ /// \brief The information of the symbol being queried.
+ std::vector<QuerySymbolInfo> QuerySymbolInfos;
+
/// \brief The symbol candidates which match SymbolIdentifier. The symbols are
/// sorted in a descending order based on the popularity info in SymbolInfo.
std::vector<find_all_symbols::SymbolInfo> MatchedSymbols;
/// \brief The header information.
std::vector<HeaderInfo> HeaderInfos;
-
- /// \brief The information of the symbol being queried.
- QuerySymbolInfo QuerySymbol;
};
} // namespace include_fixer
Index: include-fixer/IncludeFixerContext.cpp
===================================================================
--- include-fixer/IncludeFixerContext.cpp
+++ include-fixer/IncludeFixerContext.cpp
@@ -75,14 +75,33 @@
} // anonymous namespace
IncludeFixerContext::IncludeFixerContext(
- const QuerySymbolInfo &QuerySymbol,
+ std::vector<QuerySymbolInfo> QuerySymbols,
std::vector<find_all_symbols::SymbolInfo> Symbols)
- : MatchedSymbols(std::move(Symbols)), QuerySymbol(QuerySymbol) {
+ : QuerySymbolInfos(std::move(QuerySymbols)),
+ MatchedSymbols(std::move(Symbols)) {
+ // Remove replicated QuerySymbolInfos with the same range.
+ //
+ // QuerySymbolInfos may contain replicated elements. Because CorrectTypo
+ // callback doesn't always work as we expected. In somecases, it will be
+ // triggered at the same position or unidentified symbol multiple times.
+ std::sort(QuerySymbolInfos.begin(), QuerySymbolInfos.end(),
+ [&](const QuerySymbolInfo &A, const QuerySymbolInfo &B) {
+ if (A.Range.getOffset() != B.Range.getOffset())
+ return A.Range.getOffset() < B.Range.getOffset();
+ return A.Range.getLength() == B.Range.getLength();
+ });
+ QuerySymbolInfos.erase(
+ std::unique(QuerySymbolInfos.begin(), QuerySymbolInfos.end(),
+ [](const QuerySymbolInfo &A, const QuerySymbolInfo &B) {
+ return A.Range == B.Range;
+ }),
+ QuerySymbolInfos.end());
for (const auto &Symbol : MatchedSymbols) {
HeaderInfos.push_back(
{Symbol.getFilePath().str(),
createQualifiedNameForReplacement(
- QuerySymbol.RawIdentifier, QuerySymbol.ScopedQualifiers, Symbol)});
+ QuerySymbolInfos.front().RawIdentifier,
+ QuerySymbolInfos.front().ScopedQualifiers, Symbol)});
}
// Deduplicate header infos.
HeaderInfos.erase(std::unique(HeaderInfos.begin(), HeaderInfos.end(),
Index: include-fixer/IncludeFixer.cpp
===================================================================
--- include-fixer/IncludeFixer.cpp
+++ include-fixer/IncludeFixer.cpp
@@ -227,17 +227,28 @@
Symbol.getContexts(),
Symbol.getNumOccurrences());
}
- return IncludeFixerContext(QuerySymbolInfo, SymbolCandidates);
+ return IncludeFixerContext(QuerySymbolInfos, SymbolCandidates);
}
private:
/// Query the database for a given identifier.
- bool query(StringRef Query, StringRef ScopedQualifiers, tooling::Range Range) {
+ bool query(StringRef Query, StringRef ScopedQualifiers,
+ tooling::Range Range) {
assert(!Query.empty() && "Empty query!");
- // Skip other identifiers once we have discovered an identfier successfully.
- if (!MatchedSymbols.empty())
+ // Save all instances of an unidentified symbol.
+ //
+ // We use conservative behavior for detecting the same unidentified symbol
+ // here. The symbols which have the same ScopedQualifier and RawIdentifier
+ // are considered equal. So that include-fixer avoids false positives, and
+ // always adds missing qualifiers to correct symbols.
+ if (!QuerySymbolInfos.empty()) {
+ if (ScopedQualifiers.str() == QuerySymbolInfos.front().ScopedQualifiers &&
+ Query.str() == QuerySymbolInfos.front().RawIdentifier) {
+ QuerySymbolInfos.push_back({Query.str(), ScopedQualifiers, Range});
+ }
return false;
+ }
DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at ");
DEBUG(getCompilerInstance()
@@ -248,7 +259,7 @@
.print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
DEBUG(llvm::dbgs() << " ...");
- QuerySymbolInfo = {Query.str(), ScopedQualifiers, Range};
+ QuerySymbolInfos.push_back({Query.str(), ScopedQualifiers, Range});
// Query the symbol based on C++ name Lookup rules.
// Firstly, lookup the identifier with scoped namespace contexts;
@@ -274,8 +285,8 @@
/// The client to use to find cross-references.
SymbolIndexManager &SymbolIndexMgr;
- /// The symbol information.
- IncludeFixerContext::QuerySymbolInfo QuerySymbolInfo;
+ /// The information of the symbols being queried.
+ std::vector<IncludeFixerContext::QuerySymbolInfo> QuerySymbolInfos;
/// All symbol candidates which match QuerySymbol. We only include the first
/// discovered identifier to avoid getting caught in results from error
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
