[PATCH] D63254: [lit] Fix UnicodeEncodeError when test commands contain non-ASCII chars

[Michał Górny via Phabricator via llvm-commits]

mgorny marked an inline comment as done.
mgorny added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1137-1141
     if litConfig.isWindows and not isWin32CMDEXE:
-      mode += 'b'  # Avoid CRLFs when writing bash scripts.
-    f = open(script, mode)
+        mode += 'b'  # Avoid CRLFs when writing bash scripts.
+    elif sys.version_info > (3,0):
+        open_kwargs['encoding'] = 'utf-8'
+    f = open(script, mode, **open_kwargs)
----------------
ruiu wrote:
> I'd perhaps write this this way,
> 
>   f = None
>   if litConfig.isWindows and not isWin32CMDEXE:
>     f = open(script, 'wb')
>   elif sys.version_info > (3,0):
>     f = open(script, 'w', encoding='utf-8')
>   else
>     f = open(script, 'w')
> 
> so that it is clear what values are passed to `open`, but that's probably my personal preference.
Sure but the code below relies on `mode` being defined :-(. I personally think the whole function here is a horrible hack but I currently don't have a good idea how to write it better. I mean, how does it make sense to have two copies of everything with different string types in order to control endlines as a side effect? Even conditionally putting `\r\n` would be cleaner than that. In fact, maybe I'll rewrite it to do just that…


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

https://reviews.llvm.org/D63254