[PATCH] D39023: lit: Improve %: normalization.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:830
+    if kIsWindows:
+        return re.sub(r'^(.):', r'\1', path.replace('\\', '/'))
+    else:
----------------
ruiu wrote:
> This returns `/foo` for `C:\foo`, no?
I think it returns `C/foo`. The `replace` will return `C:/foo`, and then the `sub` will match the `C:` and replace it with `C`.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:870-879
+    # "%:[STpst]" are normalized paths without colons and without a leading
+    # slash. This should match the normalization of embedded paths in linkrepro
+    # tar files.
+    substitutions.extend([
+            ('%:s', colonNormalizePath(sourcepath)),
+            ('%:S', colonNormalizePath(sourcedir)),
+            ('%:p', colonNormalizePath(sourcedir)),
----------------
zturner wrote:
> I think the `%:` substitutions are used outside of linkrepro tar files, so it seems odd to change the semantics of ALL substitutions just for the purposes of a small number of tests.  
> 
> We already have `%:` (remove :), and `%/` (convert `\` to `/`).  Maybe adding `%:/` (remove : *and* convert `\` to `/`) is a better choice?  This way if for some reason someone still needs to use the existing semantics of `%:`, they can get it.
> 
> 
I couldn't find any though (did a grep for `%:` over the monorepo).


https://reviews.llvm.org/D39023