[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added projects: clang-tools-extra, All.

I think this introduced a bug: if your search path has 
`/usr/bin/../include/c++/v1`, then you'll end up with `FileEntry`s with `Name` 
like `/usr/bin/../include/c++/v1/__utility/move.h`. Calling the `FileEntry` 
overload of `suggestPathToFileForDiagnostics` won't return the relative path, 
because it will strip dots from the dir path but not the fileentry's Name.

Fix is probably to strip dots from both, I guess.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60873

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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359078: [clang][HeaderSuggestion] Handle the case of dotdot 
with an absolute path (authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60873?vs=195751=196400#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60873

Files:
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
  clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
@@ -213,6 +213,11 @@
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
 TEST_F(HeadersTest, NotShortenedInclude) {
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -1685,11 +1685,10 @@
 
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
-if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
+if (!WorkingDir.empty() && !path::is_absolute(Dir))
   fs::make_absolute(WorkingDir, DirPath);
-  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-  Dir = DirPath;
-}
+path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+Dir = DirPath;
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
  /*termination condition in loop*/; ++NI, ++DI) {
Index: cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
===
--- cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
+++ cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
@@ -100,5 +100,12 @@
 }
 #endif
 
+TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
+   /*WorkingDir=*/""),
+"z");
+}
+
 } // namespace
 } // namespace clang


Index: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
@@ -213,6 +213,11 @@
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
 TEST_F(HeadersTest, NotShortenedInclude) {
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -1685,11 +1685,10 @@
 
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
-if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
+if (!WorkingDir.empty() && !path::is_absolute(Dir))
   fs::make_absolute(WorkingDir, DirPath);
-  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-  Dir = DirPath;
-}
+path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+Dir = DirPath;
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
  /*termination condition in loop*/; ++NI, ++DI) {
Index: cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
===
--- cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
+++ cfe/trunk/unittests/Lex/HeaderSearchTest.cpp
@@ -100,5 +100,12 @@
 }
 #endif
 
+TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
+   /*WorkingDir=*/""),
+"z");
+}
+
 } // namespace
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Sent out D60995 , hopefully it should fix the 
issue. Will wait until that lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/unittests/clangd/HeadersTests.cpp:220
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }

ormris wrote:
> sammccall wrote:
> > ormris wrote:
> > > Quick nit: This test won't work on Windows as it only tests for 
> > > POSIX-style path separators. You could easily add a Windows ifdef, though.
> > testPath() returns an appropriate native path, and calculate() returns an 
> > #include string (which always uses /).
> > The "../" on line 217 looks a little more suspicious, but I think the clang 
> > driver might silently Do What I Mean here.
> > Are you seeing test failures?
> Yes, here's the output.
> 
> ```
> [ RUN  ] HeadersTest.ShortenedInclude
> C:\llvm\tools\clang\tools\extra\unittests\clangd\HeadersTests.cpp(220): 
> error:   Expected: calculate(BarHeader)
>   Which is: "\"sub\\bar.h\""
> To be equal to: "\"sub/bar.h\""
> ```
Hmm, that suggests the test is catching a real bug in the code: we should be 
inserting paths with forward slashes even on Windows.

Looking at the test cases, i have the sinking feeling we've always been doing 
this and never noticed as no test case has a slash in the final #include. (And 
none of the main clangd devs are primarily using Windows)

Thank you for pointing this out!

@kadircet: we should either fix this before this patch lands, or ifdef it until 
we can fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Matthew Voss via Phabricator via cfe-commits]

ormris added inline comments.



Comment at: clang-tools-extra/unittests/clangd/HeadersTests.cpp:220
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }

sammccall wrote:
> ormris wrote:
> > Quick nit: This test won't work on Windows as it only tests for POSIX-style 
> > path separators. You could easily add a Windows ifdef, though.
> testPath() returns an appropriate native path, and calculate() returns an 
> #include string (which always uses /).
> The "../" on line 217 looks a little more suspicious, but I think the clang 
> driver might silently Do What I Mean here.
> Are you seeing test failures?
Yes, here's the output.

```
[ RUN  ] HeadersTest.ShortenedInclude
C:\llvm\tools\clang\tools\extra\unittests\clangd\HeadersTests.cpp(220): error:  
 Expected: calculate(BarHeader)
  Which is: "\"sub\\bar.h\""
To be equal to: "\"sub/bar.h\""
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/unittests/clangd/HeadersTests.cpp:220
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }

ormris wrote:
> Quick nit: This test won't work on Windows as it only tests for POSIX-style 
> path separators. You could easily add a Windows ifdef, though.
testPath() returns an appropriate native path, and calculate() returns an 
#include string (which always uses /).
The "../" on line 217 looks a little more suspicious, but I think the clang 
driver might silently Do What I Mean here.
Are you seeing test failures?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Matthew Voss via Phabricator via cfe-commits]

ormris added inline comments.



Comment at: clang-tools-extra/unittests/clangd/HeadersTests.cpp:220
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }

Quick nit: This test won't work on Windows as it only tests for POSIX-style 
path separators. You could easily add a Windows ifdef, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Sam McCall via Phabricator via cfe-commits]

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

Well done!
I never managed to track this one down, this was really annoying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60873



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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, ioeric, ilya-biryukov.
Herald added a project: clang.

Include insertion in clangd was inserting absolute paths when the
include directory was an absolute path with a double dot. This patch makes sure
double dots are handled both with absolute and relative paths.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60873

Files:
  clang-tools-extra/unittests/clangd/HeadersTests.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp


Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -91,5 +91,12 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
+   /*WorkingDir=*/""),
+"z");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1685,11 +1685,10 @@
 
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
-if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
+if (!WorkingDir.empty() && !path::is_absolute(Dir))
   fs::make_absolute(WorkingDir, DirPath);
-  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-  Dir = DirPath;
-}
+path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+Dir = DirPath;
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
  /*termination condition in loop*/; ++NI, ++DI) {
Index: clang-tools-extra/unittests/clangd/HeadersTests.cpp
===
--- clang-tools-extra/unittests/clangd/HeadersTests.cpp
+++ clang-tools-extra/unittests/clangd/HeadersTests.cpp
@@ -213,6 +213,11 @@
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
 TEST_F(HeadersTest, NotShortenedInclude) {


Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -91,5 +91,12 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, DotDotsWithAbsPath) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z",
+   /*WorkingDir=*/""),
+"z");
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1685,11 +1685,10 @@
 
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
-if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
+if (!WorkingDir.empty() && !path::is_absolute(Dir))
   fs::make_absolute(WorkingDir, DirPath);
-  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-  Dir = DirPath;
-}
+path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+Dir = DirPath;
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
  /*termination condition in loop*/; ++NI, ++DI) {
Index: clang-tools-extra/unittests/clangd/HeadersTests.cpp
===
--- clang-tools-extra/unittests/clangd/HeadersTests.cpp
+++ clang-tools-extra/unittests/clangd/HeadersTests.cpp
@@ -213,6 +213,11 @@
 TEST_F(HeadersTest, ShortenedInclude) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
 TEST_F(HeadersTest, NotShortenedInclude) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits