[PATCH] D68033: [llvm-ar] Make paths case insensitive when on windows

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking good.



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:496
 
+static bool comparePaths(StringRef Path1, const StringRef Path2) {
+#ifndef _WIN32
----------------
I'd add a function comment here to explain what this function does for Windows. On Windows, we want a case-insensitive comparison as defined by the Unicode standard (which is I believe what CompareStringOrdinal implements), so that filenames in archives are compared case-insensitive manner.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:496
 
+static bool comparePaths(StringRef Path1, const StringRef Path2) {
+#ifndef _WIN32
----------------
ruiu wrote:
> I'd add a function comment here to explain what this function does for Windows. On Windows, we want a case-insensitive comparison as defined by the Unicode standard (which is I believe what CompareStringOrdinal implements), so that filenames in archives are compared case-insensitive manner.
nit: remove `const` from Path2.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:497
+static bool comparePaths(StringRef Path1, const StringRef Path2) {
+#ifndef _WIN32
+  return normalizePath(Path1) == normalizePath(Path2);
----------------
nit: use `ifdef` and swap the contents of then and else clauses


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

https://reviews.llvm.org/D68033