kadircet created this revision.
kadircet added a reviewer: gribozavr.
Herald added subscribers: cfe-commits, arphaman, jkorous.
Herald added a project: clang.
Currently HeaderSearch only looks at SearchDir's passed into it, but in
addition to those paths headers can be relative to including file's directory.
This patch makes sure that is taken into account.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63295
Files:
clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeFixer.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang/include/clang/Lex/HeaderSearch.h
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Sema/SemaLookup.cpp
clang/unittests/Lex/HeaderSearchTest.cpp
Index: clang/unittests/Lex/HeaderSearchTest.cpp
===================================================================
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -59,35 +59,38 @@
TEST_F(HeaderSearchTest, NoSearchDir) {
EXPECT_EQ(Search.search_dir_size(), 0u);
- EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
- "/x/y/z");
+ EXPECT_EQ(
+ Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"", ""),
+ "/x/y/z");
}
TEST_F(HeaderSearchTest, SimpleShorten) {
addSearchDir("/x");
addSearchDir("/x/y");
- EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
- "z");
+ EXPECT_EQ(
+ Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"", ""),
+ "z");
addSearchDir("/a/b/");
- EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/""),
- "c");
+ EXPECT_EQ(
+ Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/"", ""),
+ "c");
}
TEST_F(HeaderSearchTest, ShortenWithWorkingDir) {
addSearchDir("x/y");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c/x/y/z",
- /*WorkingDir=*/"/a/b/c"),
+ /*WorkingDir=*/"/a/b/c", ""),
"z");
}
TEST_F(HeaderSearchTest, Dots) {
addSearchDir("/x/./y/");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/./z",
- /*WorkingDir=*/""),
+ /*WorkingDir=*/"", ""),
"z");
addSearchDir("a/.././c/");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/m/n/./c/z",
- /*WorkingDir=*/"/m/n/"),
+ /*WorkingDir=*/"/m/n/", ""),
"z");
}
@@ -95,14 +98,15 @@
TEST_F(HeaderSearchTest, BackSlash) {
addSearchDir("C:\\x\\y\\");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
- /*WorkingDir=*/""),
+ /*WorkingDir=*/"", ""),
"z/t");
}
TEST_F(HeaderSearchTest, BackSlashWithDotDot) {
addSearchDir("..\\y");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
- /*WorkingDir=*/"C:/x/y/"),
+ /*WorkingDir=*/"C:/x/y/",
+ ""),
"z/t");
}
#endif
@@ -110,9 +114,16 @@
TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
addSearchDir("/x/../y/");
EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
- /*WorkingDir=*/""),
+ /*WorkingDir=*/"", ""),
"z");
}
+TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
+ EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+ /*WorkingDir=*/"",
+ "/y/a.cc"),
+ "z/t.h");
+}
+
} // namespace
} // namespace clang
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -5195,10 +5195,11 @@
/// Get a "quoted.h" or <angled.h> include path to use in a diagnostic
/// suggesting the addition of a #include of the specified file.
static std::string getIncludeStringForHeader(Preprocessor &PP,
- const FileEntry *E) {
- bool IsSystem;
- auto Path =
- PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, &IsSystem);
+ const FileEntry *E,
+ llvm::StringRef IncludingFile) {
+ bool IsSystem = false;
+ auto Path = PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(
+ E, IncludingFile, &IsSystem);
return (IsSystem ? '<' : '"') + Path + (IsSystem ? '>' : '"');
}
@@ -5240,6 +5241,10 @@
UniqueModules.push_back(M);
}
+ llvm::StringRef IncludingFile =
+ SourceMgr.getFileEntryForID(SourceMgr.getFileID(UseLoc))
+ ->tryGetRealPathName();
+
if (UniqueModules.empty()) {
// All candidates were global module fragments. Try to suggest a #include.
const FileEntry *E =
@@ -5248,7 +5253,7 @@
// a FixItHint there.
Diag(UseLoc, diag::err_module_unimported_use_global_module_fragment)
<< (int)MIK << Decl << !!E
- << (E ? getIncludeStringForHeader(PP, E) : "");
+ << (E ? getIncludeStringForHeader(PP, E, IncludingFile) : "");
// Produce a "previous" note if it will point to a header rather than some
// random global module fragment.
// FIXME: Suppress the note backtrace even under
@@ -5284,8 +5289,8 @@
// FIXME: Find a smart place to suggest inserting a #include, and add
// a FixItHint there.
Diag(UseLoc, diag::err_module_unimported_use_header)
- << (int)MIK << Decl << Modules[0]->getFullModuleName()
- << getIncludeStringForHeader(PP, E);
+ << (int)MIK << Decl << Modules[0]->getFullModuleName()
+ << getIncludeStringForHeader(PP, E, IncludingFile);
} else {
// FIXME: Add a FixItHint that imports the corresponding module.
Diag(UseLoc, diag::err_module_unimported_use)
Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1665,28 +1665,22 @@
SearchDir.setSearchedAllModuleMaps(true);
}
-std::string HeaderSearch::suggestPathToFileForDiagnostics(const FileEntry *File,
- bool *IsSystem) {
+std::string HeaderSearch::suggestPathToFileForDiagnostics(
+ const FileEntry *File, llvm::StringRef MainFile, bool *IsSystem) {
// FIXME: We assume that the path name currently cached in the FileEntry is
// the most appropriate one for this analysis (and that it's spelled the
// same way as the corresponding header search path).
- return suggestPathToFileForDiagnostics(File->getName(), /*BuildDir=*/"",
- IsSystem);
+ return suggestPathToFileForDiagnostics(File->getName(), /*WorkingDir=*/"",
+ MainFile, IsSystem);
}
std::string HeaderSearch::suggestPathToFileForDiagnostics(
- llvm::StringRef File, llvm::StringRef WorkingDir, bool *IsSystem) {
+ llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
+ bool *IsSystem) {
using namespace llvm::sys;
unsigned BestPrefixLength = 0;
- unsigned BestSearchDir;
-
- for (unsigned I = 0; I != SearchDirs.size(); ++I) {
- // FIXME: Support this search within frameworks and header maps.
- if (!SearchDirs[I].isNormalDir())
- continue;
-
- StringRef Dir = SearchDirs[I].getDir()->getName();
+ auto CheckDir = [&](llvm::StringRef Dir) {
llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
if (!WorkingDir.empty() && !path::is_absolute(Dir))
fs::make_absolute(WorkingDir, DirPath);
@@ -1710,7 +1704,7 @@
unsigned PrefixLength = NI - path::begin(File);
if (PrefixLength > BestPrefixLength) {
BestPrefixLength = PrefixLength;
- BestSearchDir = I;
+ return true;
}
break;
}
@@ -1723,9 +1717,21 @@
if (*NI != *DI)
break;
}
+ return false;
+ };
+
+ if (CheckDir(path::parent_path(MainFile)) && IsSystem)
+ *IsSystem = false;
+
+ for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+ // FIXME: Support this search within frameworks and header maps.
+ if (!SearchDirs[I].isNormalDir())
+ continue;
+
+ StringRef Dir = SearchDirs[I].getDir()->getName();
+ if (CheckDir(Dir) && IsSystem)
+ *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
}
- if (IsSystem)
- *IsSystem = BestPrefixLength ? BestSearchDir >= SystemDirIdx : false;
return path::convert_to_slash(File.drop_front(BestPrefixLength));
}
Index: clang/include/clang/Lex/HeaderSearch.h
===================================================================
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -709,21 +709,27 @@
/// Suggest a path by which the specified file could be found, for use in
/// diagnostics to suggest a #include. Returned path will only contain forward
- /// slashes as separators.
+ /// slashes as separators. MainFile's directory is used as another search dir,
+ /// since #includes can be relative to that directory in addition to
+ /// SearchDirs.
///
/// \param IsSystem If non-null, filled in to indicate whether the suggested
/// path is relative to a system header directory.
std::string suggestPathToFileForDiagnostics(const FileEntry *File,
+ llvm::StringRef MainFile,
bool *IsSystem = nullptr);
/// Suggest a path by which the specified file could be found, for use in
/// diagnostics to suggest a #include. Returned path will only contain forward
- /// slashes as separators.
+ /// slashes as separators. MainFile's directory is used as another search dir,
+ /// since #includes can be relative to that directory in addition to
+ /// SearchDirs.
///
/// \param WorkingDir If non-empty, this will be prepended to search directory
/// paths that are relative.
std::string suggestPathToFileForDiagnostics(llvm::StringRef File,
llvm::StringRef WorkingDir,
+ llvm::StringRef MainFile,
bool *IsSystem = nullptr);
void PrintStats();
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -97,7 +97,7 @@
auto Inserted = ToHeaderFile(Preferred);
if (!Inserter.shouldInsertInclude(Original, Inserted))
return "";
- std::string Path = Inserter.calculateIncludePath(Inserted);
+ std::string Path = Inserter.calculateIncludePath(Inserted, MainFile);
Action.EndSourceFile();
return Path;
}
@@ -180,13 +180,14 @@
// includes. (We'd test more directly, but it's pretty well encapsulated!)
auto TU = TestTU::withCode(R"cpp(
#include "a.h"
+
#include "a.h"
void foo();
#include "a.h"
)cpp");
TU.HeaderFilename = "a.h"; // suppress "not found".
EXPECT_THAT(TU.build().getIncludeStructure().MainFileIncludes,
- ElementsAre(IncludeLine(1), IncludeLine(2), IncludeLine(4)));
+ ElementsAre(IncludeLine(1), IncludeLine(3), IncludeLine(5)));
}
TEST_F(HeadersTest, UnResolvedInclusion) {
@@ -224,7 +225,7 @@
TEST_F(HeadersTest, NotShortenedInclude) {
std::string BarHeader =
llvm::sys::path::convert_to_slash(testPath("sub-2/bar.h"));
- EXPECT_EQ(calculate(BarHeader, ""), "\"" + BarHeader + "\"");
+ EXPECT_EQ(calculate(BarHeader, ""), "\"sub-2/bar.h\"");
}
TEST_F(HeadersTest, PreferredHeader) {
@@ -267,10 +268,12 @@
auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
- EXPECT_EQ(Inserter.calculateIncludePath(Inserting), "\"" + HeaderPath + "\"");
+ // FIXME(kadircet): This should result in "sub/bar.h" instead of full path.
+ EXPECT_EQ(Inserter.calculateIncludePath(Inserting, MainFile),
+ '"' + HeaderPath + '"');
EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
- EXPECT_EQ(Inserter.calculateIncludePath(Verbatim), "<x>");
+ EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile), "<x>");
EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
}
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -641,10 +641,10 @@
CodeCompleteOptions NoInsertion;
NoInsertion.InsertIncludes = CodeCompleteOptions::NeverInsert;
Results = completions(Server,
- R"cpp(
+ R"cpp(
int main() { ns::^ }
)cpp",
- {Sym}, NoInsertion);
+ {Sym}, NoInsertion);
EXPECT_THAT(Results.Completions,
ElementsAre(AllOf(Named("X"), Not(InsertInclude()))));
// Duplicate based on inclusions in preamble.
@@ -762,11 +762,7 @@
// Wait for the dynamic index being built.
ASSERT_TRUE(Server.blockUntilIdleForTest());
EXPECT_THAT(completions(Server, "Foo^ foo;").Completions,
- ElementsAre(AllOf(Named("Foo"),
- HasInclude('"' +
- llvm::sys::path::convert_to_slash(
- testPath("foo_header.h")) +
- '"'),
+ ElementsAre(AllOf(Named("Foo"), HasInclude("\"foo_header.h\""),
InsertInclude())));
}
@@ -2440,10 +2436,10 @@
/*IndexSymbols=*/{}, Options);
// Last placeholder in code patterns should be $0 to put the cursor there.
- EXPECT_THAT(
- Results.Completions,
- Contains(AllOf(Named("while"),
- SnippetSuffix(" (${1:condition}) {\n${0:statements}\n}"))));
+ EXPECT_THAT(Results.Completions,
+ Contains(AllOf(
+ Named("while"),
+ SnippetSuffix(" (${1:condition}) {\n${0:statements}\n}"))));
// However, snippets for functions must *not* end with $0.
EXPECT_THAT(Results.Completions,
Contains(AllOf(Named("while_foo"),
Index: clang-tools-extra/clangd/IncludeFixer.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeFixer.cpp
+++ clang-tools-extra/clangd/IncludeFixer.cpp
@@ -152,7 +152,7 @@
if (!ResolvedInserted)
return ResolvedInserted.takeError();
return std::make_pair(
- Inserter->calculateIncludePath(*ResolvedInserted),
+ Inserter->calculateIncludePath(*ResolvedInserted, File),
Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
};
@@ -397,7 +397,6 @@
return {};
}
-
llvm::Optional<const SymbolSlab *>
IncludeFixer::fuzzyFindCached(const FuzzyFindRequest &Req) const {
auto ReqStr = llvm::formatv("{0}", toJSON(Req)).str();
Index: clang-tools-extra/clangd/Headers.h
===================================================================
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -151,8 +151,12 @@
/// \param InsertedHeader The preferred header to be inserted. This could be
/// the same as DeclaringHeader but must be provided.
///
+ /// \param IncludingFile Used to shorten the include for headers in the same
+ /// directory.
+ ///
/// \return A quoted "path" or <path> to be included.
- std::string calculateIncludePath(const HeaderFile &InsertedHeader) const;
+ std::string calculateIncludePath(const HeaderFile &InsertedHeader,
+ llvm::StringRef IncludingFile) const;
/// Calculates an edit that inserts \p VerbatimHeader into code. If the header
/// is already included, this returns None.
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -14,6 +14,7 @@
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/FrontendActions.h"
#include "clang/Lex/HeaderSearch.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Path.h"
namespace clang {
@@ -186,15 +187,18 @@
}
std::string
-IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader) const {
+IncludeInserter::calculateIncludePath(const HeaderFile &InsertedHeader,
+ llvm::StringRef IncludingFile) const {
assert(InsertedHeader.valid());
if (InsertedHeader.Verbatim)
return InsertedHeader.File;
bool IsSystem = false;
+ // FIXME(kadircet): Handle same directory includes even if there is no
+ // HeaderSearchInfo.
if (!HeaderSearchInfo)
return "\"" + InsertedHeader.File + "\"";
std::string Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
- InsertedHeader.File, BuildDir, &IsSystem);
+ InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
if (IsSystem)
Suggested = "<" + Suggested + ">";
else
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -328,7 +328,7 @@
if (!ResolvedInserted)
return ResolvedInserted.takeError();
return std::make_pair(
- Includes.calculateIncludePath(*ResolvedInserted),
+ Includes.calculateIncludePath(*ResolvedInserted, FileName),
Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
};
bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
Index: clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
===================================================================
--- clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
+++ clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
@@ -314,9 +314,9 @@
if (!Entry)
return Include;
- bool IsSystem;
+ bool IsSystem = false;
std::string Suggestion =
- HeaderSearch.suggestPathToFileForDiagnostics(Entry, &IsSystem);
+ HeaderSearch.suggestPathToFileForDiagnostics(Entry, "", &IsSystem);
return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
