subject:"\[PATCH\] D63295\: \[clang\]\[HeaderSearch\] Shorten paths for includes in mainfile's directory"

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
Closed by commit rL365005: [clang][HeaderSearch] Shorten paths for includes in 
mainfiles directory (authored by kadircet, committed by ).
Herald added subscribers: llvm-commits, ilya-biryukov.
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D63295?vs=207497=207715#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295

Files:
  cfe/trunk/include/clang/Lex/HeaderSearch.h
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/lib/Sema/SemaLookup.cpp
  cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
  clang-tools-extra/trunk/clang-include-fixer/IncludeFixer.cpp
  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/CodeCompleteTests.cpp
  clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/trunk/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{"", /*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), "");
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim, MainFile), "");
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 }
 
Index: clang-tools-extra/trunk/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/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/trunk/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp
@@ -152,7 +152,7 @@
 if (!ResolvedInserted)
   return ResolvedInserted.takeError();
 return std::make_pair(
-Inserter->calculateIncludePath(*ResolvedInserted),
+Inserter->calculateIncludePath(*ResolvedInserted, File),

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-03 Thread Haojian Wu via Phabricator via cfe-commits

hokein accepted this revision.
hokein added inline comments.



Comment at: clang-tools-extra/clangd/Headers.h:155
+  /// \param IncludingFile is the absolute path of the file that InsertedHeader
+  /// will be instered.
+  ///

nit: instered => inserted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-02 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 207497.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Address comments


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,41 @@
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
   EXPECT_EQ(Search.search_dir_size(), 0u);
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "/x/y/z");
 }
 
 TEST_F(HeaderSearchTest, SimpleShorten) {
   addSearchDir("/x");
   addSearchDir("/x/y");
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "z");
   addSearchDir("/a/b/");
-  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/""),
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c", /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "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",
+   /*MainFile=*/""),
 "z");
 }
 
 TEST_F(HeaderSearchTest, Dots) {
   addSearchDir("/x/./y/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/./z",
-   /*WorkingDir=*/""),
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "z");
   addSearchDir("a/.././c/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/m/n/./c/z",
-   /*WorkingDir=*/"/m/n/"),
+   /*WorkingDir=*/"/m/n/",
+   /*MainFile=*/""),
 "z");
 }
 
@@ -95,14 +101,16 @@
 TEST_F(HeaderSearchTest, BackSlash) {
   addSearchDir("C:\\x\\y\\");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("C:\\x\\y\\z\\t",
-   /*WorkingDir=*/""),
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "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/",
+   /*MainFile=*/""),
 "z/t");
 }
 #endif
@@ -110,9 +118,23 @@
 TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
   addSearchDir("/x/../y/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
-   /*WorkingDir=*/""),
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
 "z");
 }
 
+TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/"/y/a.cc"),
+"z/t.h");
+
+  addSearchDir("/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
+   /*WorkingDir=*/"",
+   /*MainFile=*/"/y/a.cc"),
+"y/z/t.h");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Sema/SemaLookup.cpp
===
--- clang/lib/Sema/SemaLookup.cpp
+++

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-02 Thread Haojian Wu via Phabricator via cfe-commits

hokein added a comment.

looks mostly good, a few nits.




Comment at: clang/include/clang/Lex/HeaderSearch.h:724
+  /// slashes as separators. MainFile is the absolute path of the file that we
+  /// are generating the diagnostics for. It can be empty.
   ///

I think we should have some documentation about this new behavior in the 
comment here.



Comment at: clang/lib/Sema/SemaLookup.cpp:5245
+  llvm::StringRef IncludingFile =
+  SourceMgr.getFileEntryForID(SourceMgr.getFileID(UseLoc))
+  ->tryGetRealPathName();

it seems not safe, getFileEntryForID may return a nullptr...



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:124
+   /*WorkingDir=*/"",
+   "/y/a.cc"),
+"z/t.h");

nit: add `/*MainFile=*/` comment annotation to improve the code readability.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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  include path to use in a diagnostic
 /// suggesting the addition of a #include of the specified file.
 static std::string getIncludeStringForHeader(Preprocessor ,
- const FileEntry *E) {
-  bool IsSystem;
-  auto Path =
-  PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, );
+ const FileEntry *E,
+

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 207308.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Address comments.
- Use TUs path as a fallback, rather than a search path.


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  include path to use in a diagnostic
 /// suggesting the addition of a #include of the specified file.
 static std::string getIncludeStringForHeader(Preprocessor ,
- const FileEntry *E) {
-  bool IsSystem;
-  auto Path =
-  PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, );
+

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Haojian Wu via Phabricator via cfe-commits

hokein added a comment.

Discussed it offline, technically taking the main file directory into account 
when shorten the #include path would not break the compiler, but it may 
introduce issues violating the code style -- in clangd codebase, we prefer to 
use "#include "/header.h" style, e.g. 
`index/background.cc` uses `#include "index/Background.h"`, with this patch we 
will  shorten the include as `#include "background.h"`, so we don't count the 
the `index/background.h` as included (a new `#include "background.h"` will be 
inserted).

One possible solution is that we could use the main file directory as a last 
fallback (if we are not able to shorten the #included path using any 
search-header directories, we try to use the main-file directory).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr accepted this revision.
gribozavr added a comment.
This revision is now accepted and ready to land.

I'm not an expert in this code, but it looks reasonable.




Comment at: clang-tools-extra/clangd/Headers.h:155
+  /// \param IncludingFile Used to shorten the include for headers in the same
+  /// directory.
+  ///

Please don't write what something is used for, write what it is. You can 
explain how it affects the output of the function of course, but don't just say 
"used for" -- like what you did in clang/include/clang/Lex/HeaderSearch.h.



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:215
 
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");

This test name should probably mention the fact that the directory is on the 
include path.



Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:225
 
 TEST_F(HeadersTest, NotShortenedInclude) {
   std::string BarHeader =

The test name probably should be adjusted.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1683
   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());

I'd suggest to spell the return type, and document what it means.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-07-01 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63295/new/

https://reviews.llvm.org/D63295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

2019-06-13 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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  include path to use in a diagnostic
 /// suggesting the addition of a #include of the specified file.
 static std::string getIncludeStringForHeader(Preprocessor ,
- const FileEntry *E) {
-  bool IsSystem;
-  auto Path =
-  PP.getHeaderSearchInfo().suggestPathToFileForDiagnostics(E, );
+ const FileEntry *E,
+ llvm::StringRef IncludingFile) {
+  bool IsSystem =

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

[PATCH] D63295: [clang][HeaderSearch] Shorten paths for includes in mainfile's directory

10 matches

Site Navigation

Mail list logo

Footer information