[PATCH] D138709: Reland "[Lex] Fix suggested spelling of /usr/bin/../include/foo"

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c1fa734598c: Reland [Lex] Fix suggested spelling of 
/usr/bin/../include/foo (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D138709?vs=477937=478152#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138709

Files:
  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
@@ -150,6 +150,14 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, BothDotDots) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"z");
+}
+
 TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
/*WorkingDir=*/"",
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1928,32 +1928,28 @@
 llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
 bool *IsSystem) {
   using namespace llvm::sys;
+  
+  llvm::SmallString<32> FilePath = File;
+  // remove_dots switches to backslashes on windows as a side-effect!
+  // We always want to suggest forward slashes for includes.
+  // (not remove_dots(..., posix) as that misparses windows paths).
+  path::remove_dots(FilePath, /*remove_dot_dot=*/true);
+  path::native(FilePath, path::Style::posix);
+  File = FilePath;
 
   unsigned BestPrefixLength = 0;
   // Checks whether `Dir` is a strict path prefix of `File`. If so 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());
+  auto CheckDir = [&](llvm::SmallString<32> Dir) -> bool {
 if (!WorkingDir.empty() && !path::is_absolute(Dir))
-  fs::make_absolute(WorkingDir, DirPath);
-path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-Dir = DirPath;
+  fs::make_absolute(WorkingDir, Dir);
+path::remove_dots(Dir, /*remove_dot_dot=*/true);
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
- /*termination condition in loop*/; ++NI, ++DI) {
-  // '.' components in File are ignored.
-  while (NI != NE && *NI == ".")
-++NI;
-  if (NI == NE)
-break;
-
-  // '.' components in Dir are ignored.
-  while (DI != DE && *DI == ".")
-++DI;
+ NI != NE; ++NI, ++DI) {
   if (DI == DE) {
-// Dir is a prefix of File, up to '.' components and choice of path
-// separators.
+// Dir is a prefix of File, up to choice of path separators.
 unsigned PrefixLength = NI - path::begin(File);
 if (PrefixLength > BestPrefixLength) {
   BestPrefixLength = PrefixLength;


Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -150,6 +150,14 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, BothDotDots) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"z");
+}
+
 TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
/*WorkingDir=*/"",
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1928,32 +1928,28 @@
 llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
 bool *IsSystem) {
   using namespace llvm::sys;
+  
+  llvm::SmallString<32> FilePath = File;
+  // remove_dots switches to backslashes on windows as a side-effect!
+  // We always want to suggest forward slashes for includes.
+  // (not remove_dots(..., posix) as that misparses windows paths).
+  path::remove_dots(FilePath, /*remove_dot_dot=*/true);
+  path::native(FilePath, path::Style::posix);
+  File = FilePath;
 
   unsigned BestPrefixLength = 0;
   // Checks whether `Dir` is a 

[PATCH] D138709: Reland "[Lex] Fix suggested spelling of /usr/bin/../include/foo"

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:155
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",

kadircet wrote:
> can you also add a new test that looks like:
> ```
> addSearchDir("x/");
> EXPECT(suggestForDiag("x\y\z.h"), "y/z.h");
> ```
> 
> as in theory that's the new behaviour we're adding.
That looks a lot like `TEST_F(HeaderSearchTest, BackSlash)` to me (relative vs 
absolute, but not relevant at least to this change)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138709

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

[PATCH] D138709: Reland "[Lex] Fix suggested spelling of /usr/bin/../include/foo"

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks!




Comment at: clang/lib/Lex/HeaderSearch.cpp:1936
+  path::remove_dots(FilePath, /*remove_dot_dot=*/true);
+  path::native(FilePath, path::Style::posix);
+  File = FilePath;

might be worth adding a comment like `we can't pass posix as style to 
remove_dots, because it won't canonicalize "a\.\b.h"`



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:155
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",

can you also add a new test that looks like:
```
addSearchDir("x/");
EXPECT(suggestForDiag("x\y\z.h"), "y/z.h");
```

as in theory that's the new behaviour we're adding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138709

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

[PATCH] D138709: Reland "[Lex] Fix suggested spelling of /usr/bin/../include/foo"

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added a project: All.
sammccall requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This reverts commit 1dc0a1e5d220b83c1074204bd3afd54f3bac4270 
.

Failures were caused by unintentional conversion to native slashes by
remove_dots, so undo that: we always suggest posix slashes for includes.

This could potentially be a change in behavior on windows if people were
spelling headers with backslashes and headermaps contained backslashes,
but that's all underspecified and I don't think anyone uses headermaps
on windows.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138709

Files:
  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
@@ -150,6 +150,14 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, BothDotDots) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"z");
+}
+
 TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
/*WorkingDir=*/"",
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1928,32 +1928,27 @@
 llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
 bool *IsSystem) {
   using namespace llvm::sys;
+  
+  llvm::SmallString<32> FilePath = File;
+  // remove_dots switches to backslashes on windows as a side-effect!
+  // We always want to suggest forward slashes for includes.
+  path::remove_dots(FilePath, /*remove_dot_dot=*/true);
+  path::native(FilePath, path::Style::posix);
+  File = FilePath;
 
   unsigned BestPrefixLength = 0;
   // Checks whether `Dir` is a strict path prefix of `File`. If so 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());
+  auto CheckDir = [&](llvm::SmallString<32> Dir) -> bool {
 if (!WorkingDir.empty() && !path::is_absolute(Dir))
-  fs::make_absolute(WorkingDir, DirPath);
-path::remove_dots(DirPath, /*remove_dot_dot=*/true);
-Dir = DirPath;
+  fs::make_absolute(WorkingDir, Dir);
+path::remove_dots(Dir, /*remove_dot_dot=*/true);
 for (auto NI = path::begin(File), NE = path::end(File),
   DI = path::begin(Dir), DE = path::end(Dir);
- /*termination condition in loop*/; ++NI, ++DI) {
-  // '.' components in File are ignored.
-  while (NI != NE && *NI == ".")
-++NI;
-  if (NI == NE)
-break;
-
-  // '.' components in Dir are ignored.
-  while (DI != DE && *DI == ".")
-++DI;
+ NI != NE; ++NI, ++DI) {
   if (DI == DE) {
-// Dir is a prefix of File, up to '.' components and choice of path
-// separators.
+// Dir is a prefix of File, up to choice of path separators.
 unsigned PrefixLength = NI - path::begin(File);
 if (PrefixLength > BestPrefixLength) {
   BestPrefixLength = PrefixLength;


Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -150,6 +150,14 @@
 "z");
 }
 
+TEST_F(HeaderSearchTest, BothDotDots) {
+  addSearchDir("/x/../y/");
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z",
+   /*WorkingDir=*/"",
+   /*MainFile=*/""),
+"z");
+}
+
 TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
/*WorkingDir=*/"",
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1928,32 +1928,27 @@
 llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile,
 bool *IsSystem) {
   using namespace llvm::sys;
+  
+  llvm::SmallString<32> FilePath = File;
+  // remove_dots switches to backslashes on windows as a side-effect!
+  // We always want to suggest forward slashes for includes.
+