[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Nico Weber via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fc7a907b93a: Let normalize() for posix style convert 
backslash to slash unconditionally. (authored by thakis).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D79265?vs=261980=262180#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79265

Files:
  clang/test/Lexer/case-insensitive-include-ms.c
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,7 +1182,7 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
   Tests.emplace_back("a\\t", "a\\t", "a/t");
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -540,15 +540,9 @@
   Path = PathHome;
 }
   } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {
-auto PN = PI + 1;
-if (PN < PE && *PN == '\\')
-  ++PI; // increment once, the for loop will move over the escaped 
slash
-else
-  *PI = '/';
-  }
-}
+for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI)
+  if (*PI == '\\')
+*PI = '/';
   }
 }
 
Index: clang/test/Lexer/case-insensitive-include-ms.c
===
--- clang/test/Lexer/case-insensitive-include-ms.c
+++ clang/test/Lexer/case-insensitive-include-ms.c
@@ -6,15 +6,17 @@
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I 
%t/Output -verify
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility 
-fdiagnostics-parseable-fixits %s -include %s -I %t/Output 2>&1 | FileCheck %s
 
-// FIXME: Add a test with repeated backslashes once clang can handle that
-// in ms-compat mode on non-Windows hosts.
 #include "..\Output\.\case-insensitive-include.h"
 #include "..\Output\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\\Output\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"..Output\\.case-insensitive-include.h\""
 #include "..\output\.\case-insensitive-include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
 
 #include "apath\..\.\case-insensitive-include.h"
 #include "apath\..\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "apath\\..\\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"apath...case-insensitive-include.h\""
 #include "APath\..\.\case-insensitive-include.h" // For the sake of 
efficiency, this case is not diagnosed. :-(


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,7 +1182,7 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
   Tests.emplace_back("a\\t", "a\\t", "a/t");
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -540,15 +540,9 @@
   Path = PathHome;
 }
   } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {
-auto PN = PI + 1;
-if (PN < PE && *PN == '\\')
-  ++PI; // increment once, the for loop will move over the escaped slash
-else
-  *PI = '/';
-  }
-}
+for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI)
+  if (*PI == '\\')
+*PI = '/';
   }
 }
 
Index: clang/test/Lexer/case-insensitive-include-ms.c
===
--- clang/test/Lexer/case-insensitive-include-ms.c
+++ 

[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Seems reasonable, especially since none of the other tests break.  I don't 
fully remember the reason for the special case, and if it ever turns up, we can 
address it then.


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

https://reviews.llvm.org/D79265



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

[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Nico Weber via Phabricator via cfe-commits]

thakis updated this revision to Diff 261980.
thakis added a comment.

restore else


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

https://reviews.llvm.org/D79265

Files:
  clang/test/Lexer/case-insensitive-include-ms.c
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,9 +1182,10 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
+  Tests.emplace_back("a\\t", "a\\t", "a/t");
 
   for (auto  : Tests) {
 SmallString<64> Win(std::get<0>(T));
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -540,15 +540,9 @@
   Path = PathHome;
 }
   } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {
-auto PN = PI + 1;
-if (PN < PE && *PN == '\\')
-  ++PI; // increment once, the for loop will move over the escaped 
slash
-else
-  *PI = '/';
-  }
-}
+for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI)
+  if (*PI == '\\')
+*PI = '/';
   }
 }
 
Index: clang/test/Lexer/case-insensitive-include-ms.c
===
--- clang/test/Lexer/case-insensitive-include-ms.c
+++ clang/test/Lexer/case-insensitive-include-ms.c
@@ -6,15 +6,17 @@
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I 
%t/Output -verify
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility 
-fdiagnostics-parseable-fixits %s -include %s -I %t/Output 2>&1 | FileCheck %s
 
-// FIXME: Add a test with repeated backslashes once clang can handle that
-// in ms-compat mode on non-Windows hosts.
 #include "..\Output\.\case-insensitive-include.h"
 #include "..\Output\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\\Output\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"..Output\\.case-insensitive-include.h\""
 #include "..\output\.\case-insensitive-include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
 
 #include "apath\..\.\case-insensitive-include.h"
 #include "apath\..\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "apath\\..\\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"apath...case-insensitive-include.h\""
 #include "APath\..\.\case-insensitive-include.h" // For the sake of 
efficiency, this case is not diagnosed. :-(


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,9 +1182,10 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
+  Tests.emplace_back("a\\t", "a\\t", "a/t");
 
   for (auto  : Tests) {
 SmallString<64> Win(std::get<0>(T));
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -540,15 +540,9 @@
   Path = PathHome;
 }
   } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {
-auto PN = PI + 1;
-if (PN < PE && *PN == '\\')
-  ++PI; // increment once, the for loop will move over the escaped slash
-else
-  *PI = '/';
-  }
-}
+for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI)
+  if (*PI == '\\')
+*PI = '/';
   }
 }
 
Index: clang/test/Lexer/case-insensitive-include-ms.c
===
--- clang/test/Lexer/case-insensitive-include-ms.c
+++ clang/test/Lexer/case-insensitive-include-ms.c
@@ -6,15 +6,17 @@
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I 

[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D79265#2018770 , @compnerd wrote:

> What happens if you encounter a `"\t"` as a string?  This would convert that 
> to a `"/t"` would it not?  Although, I suppose that in practice the treatment 
> of escaped characters is not important.  I think I still prefer the `} else 
> {` here over the early return.


`"\t"` is converted to `"/t"`. That's also the behavior before this change 
though – only '\' followed by '\' was handled specially before, not '\' 
followed by anything else.

I restored the else and removed the early return.


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

https://reviews.llvm.org/D79265



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

[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

What happens if you encounter a `"\t"` as a string?  This would convert that to 
a `"/t"` would it not?  Although, I suppose that in practice the treatment of 
escaped characters is not important.  I think I still prefer the `} else {` 
here over the early return.


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

https://reviews.llvm.org/D79265



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

[PATCH] D79265: Let normalize() for posix style convert backslash to slash unconditionally.

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: compnerd.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

Currently, normalize() for posix replaces backslashes to slashes, except
that two backslashes in sequence are kept as-is.

clang calls normalize() to convert \ to / is microsoft compat mode. This
generally works well, but a path like "c:\\foo\\bar.h" with two
backslashes doesn't work due to the exception in normalize().

These paths happen naturally on Windows hosts with e.g.
`#include __FILE__`, and them not working on other hosts makes it
more difficult to write tests for this case.

The special case has been around without justification since this code
was added in r203611 (since then moved around in r215241 r215243).  No
integration tests fail if I remove it.

Try removing the special case.


https://reviews.llvm.org/D79265

Files:
  clang/test/Lexer/case-insensitive-include-ms.c
  llvm/lib/Support/Path.cpp
  llvm/unittests/Support/Path.cpp


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,7 +1182,7 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
 
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -539,17 +539,11 @@
   PathHome.append(Path.begin() + 1, Path.end());
   Path = PathHome;
 }
-  } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {
-auto PN = PI + 1;
-if (PN < PE && *PN == '\\')
-  ++PI; // increment once, the for loop will move over the escaped 
slash
-else
-  *PI = '/';
-  }
-}
+return;
   }
+  for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI)
+if (*PI == '\\')
+  *PI = '/';
 }
 
 std::string convert_to_slash(StringRef path, Style style) {
Index: clang/test/Lexer/case-insensitive-include-ms.c
===
--- clang/test/Lexer/case-insensitive-include-ms.c
+++ clang/test/Lexer/case-insensitive-include-ms.c
@@ -6,15 +6,17 @@
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I 
%t/Output -verify
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility 
-fdiagnostics-parseable-fixits %s -include %s -I %t/Output 2>&1 | FileCheck %s
 
-// FIXME: Add a test with repeated backslashes once clang can handle that
-// in ms-compat mode on non-Windows hosts.
 #include "..\Output\.\case-insensitive-include.h"
 #include "..\Output\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\\Output\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"..Output\\.case-insensitive-include.h\""
 #include "..\output\.\case-insensitive-include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
 
 #include "apath\..\.\case-insensitive-include.h"
 #include "apath\..\.\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
 // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "apath\\..\\.\\Case-Insensitive-Include.h" // expected-warning 
{{non-portable path}}
+// CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:52}:"\"apath...case-insensitive-include.h\""
 #include "APath\..\.\case-insensitive-include.h" // For the sake of 
efficiency, this case is not diagnosed. :-(


Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1182,7 +1182,7 @@
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
-  Tests.emplace_back("ab", "ab", "ab");
+  Tests.emplace_back("ab", "ab", "a//b");
   Tests.emplace_back("\\a", "\\a", "/a");
   Tests.emplace_back("a\\", "a\\", "a/");
 
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -539,17 +539,11 @@
   PathHome.append(Path.begin() + 1, Path.end());
   Path = PathHome;
 }
-  } else {
-for (auto PI = Path.begin(), PE = Path.end(); PI < PE; ++PI) {
-  if (*PI == '\\') {