hokein created this revision.
hokein added a subscriber: cfe-commits.
This is an initial version of fixing namespace issues by adding a missing
namespace prefix to an unidentified symbol.
This version only fixes the first discovered unidentified symbol
In the long run, include-fixer should fix all unidentfied symbols
with a same name at one run.
Currently, it works on command-line tool. The vim integration is not
implemented yet.
http://reviews.llvm.org/D21603
Files:
include-fixer/IncludeFixer.cpp
include-fixer/IncludeFixerContext.h
include-fixer/SymbolIndexManager.cpp
include-fixer/SymbolIndexManager.h
include-fixer/find-all-symbols/SymbolInfo.cpp
include-fixer/find-all-symbols/SymbolInfo.h
include-fixer/tool/ClangIncludeFixer.cpp
unittests/include-fixer/IncludeFixerTest.cpp
Index: unittests/include-fixer/IncludeFixerTest.cpp
===================================================================
--- unittests/include-fixer/IncludeFixerTest.cpp
+++ unittests/include-fixer/IncludeFixerTest.cpp
@@ -50,7 +50,7 @@
}
static std::string runIncludeFixer(
- StringRef Code,
+ StringRef Code, bool FixNamespace = false,
const std::vector<std::string> &ExtraArgs = std::vector<std::string>()) {
std::vector<SymbolInfo> Symbols = {
SymbolInfo("string", SymbolInfo::SymbolKind::Class, "<string>", 1,
@@ -82,14 +82,21 @@
IncludeFixerContext FixerContext;
IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
- runOnCode(&Factory, Code, "input.cc", ExtraArgs);
- if (FixerContext.Headers.empty())
+ std::string FakeFileName = "input.cc";
+ runOnCode(&Factory, Code, FakeFileName, ExtraArgs);
+ if (FixerContext.getMatchedSymbols().empty())
return Code;
tooling::Replacements Replacements =
clang::include_fixer::createInsertHeaderReplacements(
- Code, "input.cc", FixerContext.Headers.front());
+ Code, FakeFileName, FixerContext.getHeaders().front());
clang::RewriterTestContext Context;
- clang::FileID ID = Context.createInMemoryFile("input.cc", Code);
+ clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
+ if (FixNamespace && FixerContext.getSymbolRange().getLength() > 0) {
+ Replacements.emplace(
+ FakeFileName, FixerContext.getSymbolRange().getOffset(),
+ FixerContext.getSymbolRange().getLength(),
+ FixerContext.getMatchedSymbols().front().getFullyQualifiedName());
+ }
clang::tooling::applyAllReplacements(Replacements, Context.Rewrite);
return Context.getRewrittenText(ID);
}
@@ -136,20 +143,24 @@
TEST(IncludeFixer, MinimizeInclude) {
std::vector<std::string> IncludePath = {"-Idir/"};
- EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
- runIncludeFixer("a::b::foo bar;\n", IncludePath));
+ EXPECT_EQ(
+ "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+ runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
IncludePath = {"-isystemdir"};
- EXPECT_EQ("#include <otherdir/qux.h>\na::b::foo bar;\n",
- runIncludeFixer("a::b::foo bar;\n", IncludePath));
+ EXPECT_EQ(
+ "#include <otherdir/qux.h>\na::b::foo bar;\n",
+ runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
IncludePath = {"-iquotedir"};
- EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
- runIncludeFixer("a::b::foo bar;\n", IncludePath));
+ EXPECT_EQ(
+ "#include \"otherdir/qux.h\"\na::b::foo bar;\n",
+ runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
IncludePath = {"-Idir", "-Idir/otherdir"};
- EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n",
- runIncludeFixer("a::b::foo bar;\n", IncludePath));
+ EXPECT_EQ(
+ "#include \"qux.h\"\na::b::foo bar;\n",
+ runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath));
}
TEST(IncludeFixer, NestedName) {
@@ -221,6 +232,31 @@
runIncludeFixer("a::Vector v;"));
}
+TEST(IncludeFixer, FixNamespace) {
+ EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+ runIncludeFixer("b::bar b;\n", /*FixNamespace=*/true));
+
+ EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+ runIncludeFixer("a::b::bar b;\n", /*FixNamespace=*/true));
+
+ EXPECT_EQ(
+ "c::b::bar b;\n",
+ runIncludeFixer("c::b::bar b;\n", /*FixNamespace=*/true));
+
+ EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n",
+ runIncludeFixer("int test = Green;\n", /*FixNamespace=*/true));
+
+ // FIXME: Fix-namespace should not fix the global qualified identifier.
+ EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+ runIncludeFixer("::a::b::bar b;\n", /*FixNamespace=*/true));
+
+ // FIXME: Fix-namespace should not add the missing namespace prefix to the
+ // unidentified symbol which is already in that namespace.
+ EXPECT_EQ(
+ "#include \"bar.h\"\nnamespace a {\na::b::bar b;\n}\n",
+ runIncludeFixer("namespace a {\nb::bar b;\n}\n", /*FixNamespace==*/true));
+}
+
} // namespace
} // namespace include_fixer
} // namespace clang
Index: include-fixer/tool/ClangIncludeFixer.cpp
===================================================================
--- include-fixer/tool/ClangIncludeFixer.cpp
+++ include-fixer/tool/ClangIncludeFixer.cpp
@@ -16,6 +16,7 @@
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/YAMLTraits.h"
@@ -84,6 +85,11 @@
" }"),
cl::init(false), cl::cat(IncludeFixerCategory));
+cl::opt<bool> FixNamespace("fix-namespace",
+ cl::desc("Add missing namespace prefix to the "
+ "unidentified symbol automatically."),
+ cl::init(false), cl::cat(IncludeFixerCategory));
+
cl::opt<std::string> InsertHeader(
"insert-header",
cl::desc("Insert a specific header. This should run with STDIN mode.\n"
@@ -154,11 +160,11 @@
void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) {
OS << "{\n"
- " \"SymbolIdentifier\": \"" << Context.SymbolIdentifier << "\",\n"
+ " \"SymbolIdentifier\": \"" << Context.getSymbolIdentifier() << "\",\n"
" \"Headers\": [ ";
- for (const auto &Header : Context.Headers) {
+ for (const auto &Header : Context.getHeaders()) {
OS << " \"" << llvm::yaml::escape(Header) << "\"";
- if (Header != Context.Headers.back())
+ if (Header != Context.getHeaders().back())
OS << ", ";
}
OS << " ]\n"
@@ -203,14 +209,15 @@
IncludeFixerContext Context;
yin >> Context;
- if (Context.Headers.size() != 1) {
+ if (Context.getHeaders().size() != 1) {
errs() << "Expect exactly one inserted header.\n";
return 1;
}
tooling::Replacements Replacements =
clang::include_fixer::createInsertHeaderReplacements(
- Code->getBuffer(), FilePath, Context.Headers[0], InsertStyle);
+ Code->getBuffer(), FilePath, Context.getHeaders().front(),
+ InsertStyle);
std::string ChangedCode =
tooling::applyAllReplacements(Code->getBuffer(), Replacements);
llvm::outs() << ChangedCode;
@@ -239,23 +246,32 @@
return 0;
}
- if (Context.Headers.empty())
+ if (Context.getMatchedSymbols().empty())
return 0;
auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
if (!Buffer) {
errs() << "Couldn't open file: " << FilePath;
return 1;
}
- // FIXME: Rank the results and pick the best one instead of the first one.
tooling::Replacements Replacements =
clang::include_fixer::createInsertHeaderReplacements(
- /*Code=*/Buffer.get()->getBuffer(), FilePath, Context.Headers.front(),
- InsertStyle);
+ /*Code=*/Buffer.get()->getBuffer(), FilePath,
+ Context.getHeaders().front(), InsertStyle);
if (!Quiet)
- llvm::errs() << "Added #include" << Context.Headers.front();
+ llvm::errs() << "Added #include" << Context.getHeaders().front();
+
+ if (FixNamespace) {
+ const auto& Range = Context.getSymbolRange();
+ if (Range.getLength() > 0) {
+ std::string Name =
+ Context.getMatchedSymbols().front().getFullyQualifiedName();
+ Replacements.emplace(FilePath, Range.getOffset(), Range.getLength(),
+ Name);
+ }
+ }
// Set up a new source manager for applying the resulting replacements.
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
Index: include-fixer/find-all-symbols/SymbolInfo.h
===================================================================
--- include-fixer/find-all-symbols/SymbolInfo.h
+++ include-fixer/find-all-symbols/SymbolInfo.h
@@ -57,6 +57,9 @@
/// \brief Get symbol name.
llvm::StringRef getName() const { return Name; }
+ /// \brief Get the fully-qualified symbol name.
+ std::string getFullyQualifiedName() const;
+
/// \brief Get symbol type.
SymbolKind getSymbolKind() const { return Type; }
Index: include-fixer/find-all-symbols/SymbolInfo.cpp
===================================================================
--- include-fixer/find-all-symbols/SymbolInfo.cpp
+++ include-fixer/find-all-symbols/SymbolInfo.cpp
@@ -90,6 +90,16 @@
Symbol.Contexts);
}
+std::string SymbolInfo::getFullyQualifiedName() const {
+ std::string FullyQuanlifiedName = Name;
+ for (const auto &Context : Contexts) {
+ if (Context.first == ContextType::EnumDecl)
+ continue;
+ FullyQuanlifiedName = Context.second + "::" + FullyQuanlifiedName;
+ }
+ return FullyQuanlifiedName;
+}
+
bool WriteSymbolInfosToStream(llvm::raw_ostream &OS,
const std::set<SymbolInfo> &Symbols) {
llvm::yaml::Output yout(OS);
Index: include-fixer/SymbolIndexManager.h
===================================================================
--- include-fixer/SymbolIndexManager.h
+++ include-fixer/SymbolIndexManager.h
@@ -11,6 +11,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_SYMBOLINDEXMANAGER_H
#include "SymbolIndex.h"
+#include "find-all-symbols/SymbolInfo.h"
#include "llvm/ADT/StringRef.h"
namespace clang {
@@ -29,11 +30,10 @@
/// fully qualified.
/// \returns A list of inclusion candidates, in a format ready for being
/// pasted after an #include token.
- // FIXME: Expose the type name so we can also insert using declarations (or
- // fix the usage)
// FIXME: Move mapping from SymbolInfo to headers out of
// SymbolIndexManager::search and return SymbolInfos instead of header paths.
- std::vector<std::string> search(llvm::StringRef Identifier) const;
+ std::vector<find_all_symbols::SymbolInfo>
+ search(llvm::StringRef Identifier) const;
private:
std::vector<std::unique_ptr<SymbolIndex>> SymbolIndices;
Index: include-fixer/SymbolIndexManager.cpp
===================================================================
--- include-fixer/SymbolIndexManager.cpp
+++ include-fixer/SymbolIndexManager.cpp
@@ -20,7 +20,7 @@
using clang::find_all_symbols::SymbolInfo;
-/// Sorts and uniques SymbolInfos based on the popularity info in SymbolInfo.
+/// Sorts SymbolInfos based on the popularity info in SymbolInfo.
static void rankByPopularity(std::vector<SymbolInfo> &Symbols) {
// First collect occurrences per header file.
llvm::DenseMap<llvm::StringRef, unsigned> HeaderPopularity;
@@ -39,17 +39,9 @@
return APop > BPop;
return A.getFilePath() < B.getFilePath();
});
-
- // Deduplicate based on the file name. They will have the same popularity and
- // we don't want to suggest the same header twice.
- Symbols.erase(std::unique(Symbols.begin(), Symbols.end(),
- [](const SymbolInfo &A, const SymbolInfo &B) {
- return A.getFilePath() == B.getFilePath();
- }),
- Symbols.end());
}
-std::vector<std::string>
+std::vector<find_all_symbols::SymbolInfo>
SymbolIndexManager::search(llvm::StringRef Identifier) const {
// The identifier may be fully qualified, so split it and get all the context
// names.
@@ -127,20 +119,7 @@
}
rankByPopularity(MatchedSymbols);
- std::vector<std::string> Results;
- for (const auto &Symbol : MatchedSymbols) {
- // FIXME: file path should never be in the form of <...> or "...", but
- // the unit test with fixed database use <...> file path, which might
- // need to be changed.
- // FIXME: if the file path is a system header name, we want to use
- // angle brackets.
- std::string FilePath = Symbol.getFilePath().str();
- Results.push_back((FilePath[0] == '"' || FilePath[0] == '<')
- ? FilePath
- : "\"" + FilePath + "\"");
- }
-
- return Results;
+ return MatchedSymbols;
}
} // namespace include_fixer
Index: include-fixer/IncludeFixerContext.h
===================================================================
--- include-fixer/IncludeFixerContext.h
+++ include-fixer/IncludeFixerContext.h
@@ -10,18 +10,64 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H
#define LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H
+#include "find-all-symbols/SymbolInfo.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include <algorithm>
#include <string>
#include <vector>
namespace clang {
namespace include_fixer {
/// \brief A context for the symbol being queried.
-struct IncludeFixerContext {
+class IncludeFixerContext {
+public:
+ IncludeFixerContext() {}
+ IncludeFixerContext(llvm::StringRef Name,
+ const std::vector<find_all_symbols::SymbolInfo> Symbols,
+ tooling::Range Range)
+ : SymbolIdentifier(Name), 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());
+ }
+
+ /// \brief Get symbol name.
+ llvm::StringRef getSymbolIdentifier() const { return SymbolIdentifier; }
+
+ /// \brief Get replacement range of the symbol.
+ tooling::Range getSymbolRange() const { return SymbolRange; }
+
+ /// \brief Get all matched symbols.
+ const std::vector<find_all_symbols::SymbolInfo> &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<std::string> &getHeaders() const { return Headers; }
+
+private:
+ friend struct llvm::yaml::MappingTraits<IncludeFixerContext>;
+
/// \brief The symbol name.
std::string SymbolIdentifier;
+
/// \brief The headers which have SymbolIdentifier definitions.
std::vector<std::string> 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<find_all_symbols::SymbolInfo> MatchedSymbols;
+
+ /// \breif The replacement range of SymbolIdentifier.
+ tooling::Range SymbolRange;
};
} // namespace include_fixer
Index: include-fixer/IncludeFixer.cpp
===================================================================
--- include-fixer/IncludeFixer.cpp
+++ include-fixer/IncludeFixer.cpp
@@ -70,7 +70,11 @@
return false;
clang::ASTContext &context = getCompilerInstance().getASTContext();
- query(T.getUnqualifiedType().getAsString(context.getPrintingPolicy()), Loc);
+ llvm::StringRef QueryString =
+ T.getUnqualifiedType().getAsString(context.getPrintingPolicy());
+ DEBUG(llvm::dbgs() << "Query missing complete type '" << QueryString
+ << "'");
+ query(QueryString, tooling::Range());
return false;
}
@@ -152,18 +156,26 @@
/// If we have a scope specification, use that to get more precise results.
std::string QueryString;
+ tooling::Range SymbolRange;
+ const auto &SM = getCompilerInstance().getSourceManager();
if (SS && SS->getRange().isValid()) {
auto Range = CharSourceRange::getTokenRange(SS->getRange().getBegin(),
Typo.getLoc());
QueryString = ExtendNestedNameSpecifier(Range);
+ SymbolRange = tooling::Range(SM.getDecomposedLoc(Range.getBegin()).second,
+ QueryString.size());
} else if (Typo.getName().isIdentifier() && !Typo.getLoc().isMacroID()) {
auto Range =
CharSourceRange::getTokenRange(Typo.getBeginLoc(), Typo.getEndLoc());
QueryString = ExtendNestedNameSpecifier(Range);
+ SymbolRange = tooling::Range(SM.getDecomposedLoc(Range.getBegin()).second,
+ QueryString.size());
} else {
QueryString = Typo.getAsString();
+ SymbolRange = tooling::Range(SM.getDecomposedLoc(Typo.getLoc()).second,
+ QueryString.size());
}
// Follow C++ Lookup rules. Firstly, lookup the identifier with scoped
@@ -176,8 +188,8 @@
//
// 1. lookup a::b::foo.
// 2. lookup b::foo.
- if (!query(TypoScopeString + QueryString, Typo.getLoc()))
- query(QueryString, Typo.getLoc());
+ if (!query(TypoScopeString + QueryString, SymbolRange))
+ query(QueryString, SymbolRange);
// FIXME: We should just return the name we got as input here and prevent
// clang from trying to correct the typo by itself. That may change the
@@ -215,32 +227,45 @@
IncludeFixerContext
getIncludeFixerContext(const clang::SourceManager &SourceManager,
clang::HeaderSearch &HeaderSearch) {
- IncludeFixerContext FixerContext;
- FixerContext.SymbolIdentifier = QuerySymbol;
- for (const auto &Header : SymbolQueryResults)
- FixerContext.Headers.push_back(
- minimizeInclude(Header, SourceManager, HeaderSearch));
-
- return FixerContext;
+ std::vector<find_all_symbols::SymbolInfo> SymbolCandidates;
+ for (const auto &Symbol : MatchedSymbols) {
+ std::string FilePath = Symbol.getFilePath().str();
+ std::string MinimizedFilePath = minimizeInclude(
+ ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
+ : "\"" + FilePath + "\""),
+ SourceManager, HeaderSearch);
+ SymbolCandidates.emplace_back(Symbol.getName(), Symbol.getSymbolKind(),
+ MinimizedFilePath, Symbol.getLineNumber(),
+ Symbol.getContexts(),
+ Symbol.getNumOccurrences());
+ }
+ return IncludeFixerContext(QuerySymbol, SymbolCandidates, QuerySymbolRange);
}
private:
/// Query the database for a given identifier.
- bool query(StringRef Query, SourceLocation Loc) {
+ bool query(StringRef Query, tooling::Range Range) {
assert(!Query.empty() && "Empty query!");
- // Skip other identifers once we have discovered an identfier successfully.
- if (!SymbolQueryResults.empty())
+ // Skip other identifiers once we have discovered an identfier successfully.
+ if (!MatchedSymbols.empty())
return false;
DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at ");
- DEBUG(Loc.print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
+ DEBUG(getCompilerInstance()
+ .getSourceManager()
+ .getLocForStartOfFile(
+ getCompilerInstance().getSourceManager().getMainFileID())
+ .getLocWithOffset(Range.getOffset())
+ .print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
DEBUG(llvm::dbgs() << " ...");
QuerySymbol = Query.str();
- SymbolQueryResults = SymbolIndexMgr.search(Query);
- DEBUG(llvm::dbgs() << SymbolQueryResults.size() << " replies\n");
- return !SymbolQueryResults.empty();
+ QuerySymbolRange = Range;
+ MatchedSymbols = SymbolIndexMgr.search(Query);
+ DEBUG(llvm::dbgs() << "Having found " << MatchedSymbols.size()
+ << " symbols\n");
+ return !MatchedSymbols.empty();
}
/// The client to use to find cross-references.
@@ -252,9 +277,13 @@
/// The symbol being queried.
std::string QuerySymbol;
- /// The query results of an identifier. We only include the first discovered
- /// identifier to avoid getting caught in results from error recovery.
- std::vector<std::string> SymbolQueryResults;
+ /// The repacement range of the first discovered QuerySymbol.
+ tooling::Range QuerySymbolRange;
+
+ /// All symbol candidates which match QuerySymbol. We only include the first
+ /// discovered identifier to avoid getting caught in results from error
+ /// recovery.
+ std::vector<find_all_symbols::SymbolInfo> MatchedSymbols;
/// Whether we should use the smallest possible include path.
bool MinimizeIncludePaths = true;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
