[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: hans.
Herald added subscribers: llvm-commits, mgorny.
Herald added a project: LLVM.

See PR45812 for motivation.

No explicit test since I couldn't figure out how to get the
current disk drive in lower case into a form in lit where I could
mkdir it and cd to it. But the change does have test coverage in
that I can remove the case normalization in lit, and tests failed
on several bots (and for me locally if in a pwd with a lower-case
drive) without that normalization prior to this change.


https://reviews.llvm.org/D79531

Files:
  clang/lib/Lex/PPDirectives.cpp
  llvm/cmake/modules/AddLLVM.cmake


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1499,7 +1499,6 @@
   "def path(p):\n"
   "if not p: return ''\n"
   "p = os.path.join(os.path.dirname(os.path.abspath(__file__)), p)\n"
-  "if os.name == 'nt' and os.path.isabs(p): return p[0].upper() + p[1:]\n"
   "return p\n"
   )
 
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2104,6 +2104,27 @@
 StringRef RealPathName = File->getFileEntry().tryGetRealPathName();
 SmallVector Components(llvm::sys::path::begin(Name),
   llvm::sys::path::end(Name));
+#if defined(_WIN32)
+// -Wnonportable-include-path is designed to diagnose includes using
+// case even on systems with a case-insensitive file system.
+// On Windows, RealPathName always starts with an upper-case drive
+// letter for absolute paths, but Name might start with either
+// case depending on if `cd c:\foo` or `cd C:\foo` was used.
+// ("foo" will always have on-disk case, no matter which case was
+// used in the cd command). To not emit this warning solely for
+// the drive letter, whose case is dependent on if `cd` is used
+// with upper- or lower-case drive letters, always consider the
+// given drive letter case as correct for the purpose of this warning.
+SmallString<128> FixedDriveRealPath;
+if (llvm::sys::path::is_absolute(Name) &&
+llvm::sys::path::is_absolute(RealPathName) &&
+toLowercase(Name[0]) == toLowercase(RealPathName[0]) &&
+isLowercase(Name[0]) != isLowercase(RealPathName[0])) {
+  assert(Components.size() >= 3 && "should have drive, backslash, name");
+  FixedDriveRealPath = (Name.substr(0, 1) + RealPathName.substr(1)).str();
+  RealPathName = FixedDriveRealPath;
+}
+#endif
 
 if (trySimplifyPath(Components, RealPathName)) {
   SmallString<128> Path;


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1499,7 +1499,6 @@
   "def path(p):\n"
   "if not p: return ''\n"
   "p = os.path.join(os.path.dirname(os.path.abspath(__file__)), p)\n"
-  "if os.name == 'nt' and os.path.isabs(p): return p[0].upper() + p[1:]\n"
   "return p\n"
   )
 
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2104,6 +2104,27 @@
 StringRef RealPathName = File->getFileEntry().tryGetRealPathName();
 SmallVector Components(llvm::sys::path::begin(Name),
   llvm::sys::path::end(Name));
+#if defined(_WIN32)
+// -Wnonportable-include-path is designed to diagnose includes using
+// case even on systems with a case-insensitive file system.
+// On Windows, RealPathName always starts with an upper-case drive
+// letter for absolute paths, but Name might start with either
+// case depending on if `cd c:\foo` or `cd C:\foo` was used.
+// ("foo" will always have on-disk case, no matter which case was
+// used in the cd command). To not emit this warning solely for
+// the drive letter, whose case is dependent on if `cd` is used
+// with upper- or lower-case drive letters, always consider the
+// given drive letter case as correct for the purpose of this warning.
+SmallString<128> FixedDriveRealPath;
+if (llvm::sys::path::is_absolute(Name) &&
+llvm::sys::path::is_absolute(RealPathName) &&
+toLowercase(Name[0]) == toLowercase(RealPathName[0]) &&
+isLowercase(Name[0]) != isLowercase(RealPathName[0])) {
+  assert(Components.size() >= 3 && "should have drive, backslash, name");
+  FixedDriveRealPath = (Name.substr(0, 1) + RealPathName.substr(1)).str();
+  RealPathName = FixedDriveRealPath;
+}
+#endif
 
 if (trySimplifyPath(Components, RealPathName)) {
   SmallString<128> Path;
___
cfe-commits maili

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:2118
+// given drive letter case as correct for the purpose of this warning.
+SmallString<128> FixedDriveRealPath;
+if (llvm::sys::path::is_absolute(Name) &&

It seems like trySimplifyPath could check if it is checking the case of the 
first component on Windows, and that would hide the complexity from the main 
Preprocessor::HandleHeaderIncludeOrImport implementation. It also saves a 
string copy for every include.


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

https://reviews.llvm.org/D79531



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

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Nico Weber via Phabricator via cfe-commits]

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



Comment at: clang/lib/Lex/PPDirectives.cpp:2118
+// given drive letter case as correct for the purpose of this warning.
+SmallString<128> FixedDriveRealPath;
+if (llvm::sys::path::is_absolute(Name) &&

rnk wrote:
> It seems like trySimplifyPath could check if it is checking the case of the 
> first component on Windows, and that would hide the complexity from the main 
> Preprocessor::HandleHeaderIncludeOrImport implementation. It also saves a 
> string copy for every include.
Not easily, since that walks the path backwards. Includes are almost never 
absolute, so this also almost never does a string copy and it's imho a bit 
simpler this way.

I agree pulling out the code for this warning into its own function is a good 
idea though.


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

https://reviews.llvm.org/D79531



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

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Hans Wennborg via Phabricator via cfe-commits]

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

Looks reasonable to me, but Windows paths are scary..




Comment at: clang/lib/Lex/PPDirectives.cpp:2112
+// letter for absolute paths, but Name might start with either
+// case depending on if `cd c:\foo` or `cd C:\foo` was used.
+// ("foo" will always have on-disk case, no matter which case was

Maybe expand "was used" to "was used in the shell" or something. It may not be 
clear for people without context, since this whole thing is pretty obscure.



Comment at: clang/lib/Lex/PPDirectives.cpp:2123
+isLowercase(Name[0]) != isLowercase(RealPathName[0])) {
+  assert(Components.size() >= 3 && "should have drive, backslash, name");
+  FixedDriveRealPath = (Name.substr(0, 1) + RealPathName.substr(1)).str();

Could it be different for e.g. network drives? I guess maybe they'd still have 
at least 3 components, but perhaps no drive letter


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

https://reviews.llvm.org/D79531



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

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Nico Weber via Phabricator via cfe-commits]

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



Comment at: clang/lib/Lex/PPDirectives.cpp:2123
+isLowercase(Name[0]) != isLowercase(RealPathName[0])) {
+  assert(Components.size() >= 3 && "should have drive, backslash, name");
+  FixedDriveRealPath = (Name.substr(0, 1) + RealPathName.substr(1)).str();

hans wrote:
> Could it be different for e.g. network drives? I guess maybe they'd still 
> have at least 3 components, but perhaps no drive letter
Oh, good call, ` /FI\\?\%cd%\test.h` produces a path that's is_absolute() but 
that returns

```
\\?
\
c:
\
src
llvm-project
test.h
```

as path components (one per line). Looking at how to handle that now. If anyone 
happens to know, please shout :)


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

https://reviews.llvm.org/D79531



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

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

[Nico Weber via Phabricator via cfe-commits]

thakis closed this revision.
thakis marked an inline comment as done.
thakis added a comment.

Landed in d03838343f2199580 
. Found 
another bug elsewhere while looking at this, will make a patch for that now.




Comment at: clang/lib/Lex/PPDirectives.cpp:2123
+isLowercase(Name[0]) != isLowercase(RealPathName[0])) {
+  assert(Components.size() >= 3 && "should have drive, backslash, name");
+  FixedDriveRealPath = (Name.substr(0, 1) + RealPathName.substr(1)).str();

thakis wrote:
> hans wrote:
> > Could it be different for e.g. network drives? I guess maybe they'd still 
> > have at least 3 components, but perhaps no drive letter
> Oh, good call, ` /FI\\?\%cd%\test.h` produces a path that's is_absolute() but 
> that returns
> 
> ```
> \\?
> \
> c:
> \
> src
> llvm-project
> test.h
> ```
> 
> as path components (one per line). Looking at how to handle that now. If 
> anyone happens to know, please shout :)
This now strips the common UNC path at the start and restores it at the end if 
it was there, and there's a test for this.


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

https://reviews.llvm.org/D79531



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