[PATCH] D102339: [clang] On Windows, ignore case and separators when discarding duplicate dependency file paths.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.


================
Comment at: clang/lib/Frontend/DependencyFile.cpp:145
+  StringRef SearchPath;
+#ifdef _WIN32
+  // Make the search insensitive to case and separators.
----------------
rnk wrote:
> I feel like this should somehow be a property of the input virtual filesystem: we should be able to ask the VFS to do the path canonicalization for us, or ask if the FS is case insensitive and do the lower-casing if so.
Additional complication:

Windows NTFS partitions can have case-sensitivity enabled on a per-directory basis, so there is no single answer for the filesystem or even the host OS.  Granted, this is not yet a commonly used feature.

I wonder about the (also rarely used) case of cross compiling for a Linux target on a Windows build machine.


================
Comment at: clang/lib/Frontend/DependencyFile.cpp:149
+  std::replace(TmpPath.begin(), TmpPath.end(), '/', '\\');
+  std::transform(TmpPath.begin(), TmpPath.end(), TmpPath.begin(), ::tolower);
+  SearchPath = TmpPath.str();
----------------
If `::tolower` is the same as `std::tolower` (e.g., if the header declared it in both the `std` and global namespaces), and if the locale is the default "C" locale, then this downcases just the 26 ASCII capital letters.  Windows considers more characters when it's wearing its case-blinding glasses. 
 For example, the pre-composed `U+00C1 LATIN CAPITAL LETTER A WITH ACUTE ACCENT` and `U+00E1 LATIN SMALL LETTER A WITH ACUTE ACCENT` characters match in case-blind file names.

I'll concede that this is probably an existing problem elsewhere in LLVM, so it may not be a high enough priority to worry about right now.  It just underscores the need for better centralized handling of paths and file names, so that we'll have a handle these sorts of problems if/when we ever accept that Windows is not Posix with backward slashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102339