kadircet updated this revision to Diff 207312.
kadircet added a comment.
- Add comments to the CheckDir lambda.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63295/new/
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,22 @@
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");
+
+ addSearchDir("/");
+ EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+ /*WorkingDir=*/"",
+ "/y/a.cc"),
+ "y/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,25 @@
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();
+ // Checks whether Dir and File shares a common prefix, if they do and that's
+ // the longest prefix we've seen so for it returns true and updates the
+ // BestPrefixLength accordingly.
+ auto CheckDir = [&](llvm::StringRef Dir) -> bool {
llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
if (!WorkingDir.empty() && !path::is_absolute(Dir))
fs::make_absolute(WorkingDir, DirPath);
@@ -1710,7 +1707,7 @@
unsigned PrefixLength = NI - path::begin(File);
if (PrefixLength > BestPrefixLength) {
BestPrefixLength = PrefixLength;
- BestSearchDir = I;
+ return true;
}
break;
}
@@ -1723,9 +1720,24 @@
if (*NI != *DI)
break;
}
+ return 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;
+ // Try to shorten include path using TUs directory, if we couldn't find any
+ // suitable prefix in include search paths.
+ if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
+ *IsSystem = 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,25 @@
/// 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 is the absolute path of the file that we
+ /// are generating the diagnostics for. It can be empty.
///
/// \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 is the absolute path of the file that we
+ /// are generating the diagnostics for. It can be empty.
///
/// \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) {
@@ -211,7 +212,7 @@
EXPECT_EQ(calculate(MainFile), "");
}
-TEST_F(HeadersTest, ShortenedInclude) {
+TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
std::string BarHeader = testPath("sub/bar.h");
EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
@@ -221,10 +222,10 @@
EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
}
-TEST_F(HeadersTest, NotShortenedInclude) {
+TEST_F(HeadersTest, ShortenedIncludeNotInSearchPath) {
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
@@ -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())));
}
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 is the absolute path of the file that InsertedHeader
+ /// will be instered.
+ ///
/// \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
