[clang-tools-extra] r365364 - [clangd] Don't insert absolute paths, give up instead.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 8 11:07:46 PDT 2019
Author: sammccall
Date: Mon Jul 8 11:07:46 2019
New Revision: 365364
URL: http://llvm.org/viewvc/llvm-project?rev=365364&view=rev
Log:
[clangd] Don't insert absolute paths, give up instead.
Summary: Also implement resolution of paths relative to mainfile without HeaderSearchInfo.
Reviewers: kadircet
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D64293
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Headers.cpp
clang-tools-extra/trunk/clangd/Headers.h
clang-tools-extra/trunk/clangd/IncludeFixer.cpp
clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=365364&r1=365363&r2=365364&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Mon Jul 8 11:07:46 2019
@@ -327,8 +327,12 @@ struct CodeCompletionBuilder {
auto ResolvedInserted = toHeaderFile(Header, FileName);
if (!ResolvedInserted)
return ResolvedInserted.takeError();
+ auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName);
+ if (!Spelled)
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Header not on include path");
return std::make_pair(
- Includes.calculateIncludePath(*ResolvedInserted, FileName),
+ std::move(*Spelled),
Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
};
bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
Modified: clang-tools-extra/trunk/clangd/Headers.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Headers.cpp?rev=365364&r1=365363&r2=365364&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Headers.cpp (original)
+++ clang-tools-extra/trunk/clangd/Headers.cpp Mon Jul 8 11:07:46 2019
@@ -186,19 +186,29 @@ bool IncludeInserter::shouldInsertInclud
return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
}
-std::string
+llvm::Optional<std::string>
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, IncludingFile, &IsSystem);
+ std::string Suggested;
+ if (HeaderSearchInfo) {
+ Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
+ InsertedHeader.File, BuildDir, IncludingFile, &IsSystem);
+ } else {
+ // Calculate include relative to including file only.
+ StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
+ SmallString<256> RelFile(InsertedHeader.File);
+ // Replacing with "" leaves "/RelFile" if IncludingDir doesn't end in "/".
+ llvm::sys::path::replace_path_prefix(RelFile, IncludingDir, "./");
+ Suggested = llvm::sys::path::convert_to_slash(
+ llvm::sys::path::remove_leading_dotslash(RelFile));
+ }
+ // FIXME: should we allow (some limited number of) "../header.h"?
+ if (llvm::sys::path::is_absolute(Suggested))
+ return None;
if (IsSystem)
Suggested = "<" + Suggested + ">";
else
Modified: clang-tools-extra/trunk/clangd/Headers.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Headers.h?rev=365364&r1=365363&r2=365364&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Headers.h (original)
+++ clang-tools-extra/trunk/clangd/Headers.h Mon Jul 8 11:07:46 2019
@@ -171,15 +171,16 @@ public:
/// search path. Usually this will prefer a shorter representation like
/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
///
- /// \param InsertedHeader The preferred header to be inserted. This could be
- /// the same as DeclaringHeader but must be provided.
+ /// \param InsertedHeader The preferred header to be inserted.
///
/// \param IncludingFile is the absolute path of the file that InsertedHeader
/// will be inserted.
///
- /// \return A quoted "path" or <path> to be included.
- std::string calculateIncludePath(const HeaderFile &InsertedHeader,
- llvm::StringRef IncludingFile) const;
+ /// \return A quoted "path" or <path> to be included, or None if it couldn't
+ /// be shortened.
+ llvm::Optional<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.
Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=365364&r1=365363&r2=365364&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Mon Jul 8 11:07:46 2019
@@ -151,8 +151,12 @@ std::vector<Fix> IncludeFixer::fixesForS
auto ResolvedInserted = toHeaderFile(Header, File);
if (!ResolvedInserted)
return ResolvedInserted.takeError();
+ auto Spelled = Inserter->calculateIncludePath(*ResolvedInserted, File);
+ if (!Spelled)
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Header not on include path");
return std::make_pair(
- Inserter->calculateIncludePath(*ResolvedInserted, File),
+ std::move(*Spelled),
Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
};
Modified: clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp?rev=365364&r1=365363&r2=365364&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp Mon Jul 8 11:07:46 2019
@@ -97,9 +97,9 @@ protected:
auto Inserted = ToHeaderFile(Preferred);
if (!Inserter.shouldInsertInclude(Original, Inserted))
return "";
- std::string Path = Inserter.calculateIncludePath(Inserted, MainFile);
+ auto Path = Inserter.calculateIncludePath(Inserted, MainFile);
Action.EndSourceFile();
- return Path;
+ return Path.getValueOr("");
}
llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader) {
@@ -212,6 +212,11 @@ TEST_F(HeadersTest, DoNotInsertIfInSameF
EXPECT_EQ(calculate(MainFile), "");
}
+TEST_F(HeadersTest, DoNotInsertOffIncludePath) {
+ MainFile = testPath("sub/main.cpp");
+ EXPECT_EQ(calculate(testPath("sub2/main.cpp")), "");
+}
+
TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
std::string BarHeader = testPath("sub/bar.h");
EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
@@ -268,13 +273,16 @@ TEST(Headers, NoHeaderSearchInfo) {
auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
- // FIXME(kadircet): This should result in "sub/bar.h" instead of full path.
EXPECT_EQ(Inserter.calculateIncludePath(Inserting, MainFile),
- '"' + HeaderPath + '"');
+ std::string("\"sub/bar.h\""));
EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
- EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile), "<x>");
+ EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile),
+ std::string("<x>"));
EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
+
+ EXPECT_EQ(Inserter.calculateIncludePath(Inserting, "sub2/main2.cpp"),
+ llvm::None);
}
} // namespace
More information about the cfe-commits
mailing list