[libcxx-commits] [PATCH] D120546: [libcxx] [test] Fix dsl.sh.py to work on Windows.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/selftest/dsl/dsl.sh.py:252-256
+        # The original spelling here, "for_sure_this_is_not_an_existing_locale",
+        # is accidentally detected as existing by the Windows UCRT.
+        # https://developercommunity.visualstudio.com/t/setlocale-succeeds-for-bogus-locale-names-in-older/1652241
+        # This is fixed in UCRT/Windows 10.0.18362.0.
+        self.assertFalse(dsl.hasAnyLocale(self.config, ['forsurethisisnotanexistinglocale']))
----------------
Please remove the comment, and just mention it (including that URL) in the commit message.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:174-177
     actualOut = re.search("# command output:\n(.+)\n$", out, flags=re.DOTALL)
     actualOut = actualOut.group(1) if actualOut else ""
+    if platform.system() == 'Windows':
+      actualOut = actualOut.replace('\r\n', '\n')
----------------
I'm mildly confused how the `re.search` on line 174 could ever succeed, if `out` always contains `\r\n` in place of `\n`. Patterns treat `\n` as magic, maybe?
If we were trying to deal with `\r\n` in generated files, we'd do, like,
```
with open(out_path, 'w', newline='\n') as f:
                         ^^^^^^^^^^^^
```
I'd like (you) to explore whether we can similarly push the `\r\n` handling all the way down into the library layer here. Should we delegate the `\r\n` handling down into `_executeScriptInternal`? Should we delegate it down into whatever `_executeScriptInternal` calls? Etc. Maybe it ultimately ends up that all we need to do is change `subprocess.call(~~~)` into `subprocess.call(~~~, translateNewlines=True)` or something; that would be awesome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120546