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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I am probably not the best person to review it (I know little about llvm-ar code).
But anyways, my comments are below. (Generally looks good to me).



================
Comment at: llvm/test/tools/llvm-ar/windows-case.test:1
+## Test that on windows, members are case insensitive.
+# REQUIRES: system-windows
----------------
'windows-case.test' sounds not so clear for me. Perhaps `windows-name-case.test` would be better?

Do we have something like this test for non-windows, btw? Would be probably nice to show the behavior
of the same cases, but on a case sensitive OS.


================
Comment at: llvm/test/tools/llvm-ar/windows-case.test:18
+# RUN: llvm-ar Trc %t/thin-archive.a %t/lowerCase/file.txt %t/UPPERCASE/FILE.txt
+# RUN: llvm-ar dTP %t/thin-archive.a %t/uppercase/file.txt
+
----------------
`T` is create a thin archive, why do you need it here? (the test passes without it).


================
Comment at: llvm/test/tools/llvm-ar/windows-case.test:23
+
+# RUN: llvm-ar dTP %t/thin-archive.a %t/LOWERCASE/FILE.TXT
+# RUN: llvm-ar t %t/thin-archive.a | count 0
----------------
The same.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:494
+                   NormalizedPath.begin(), ::toupper);
+  }
+
----------------
You do not need wrapping this piece into curly bracers.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68033