[PATCH] D35093: [lit] Implement non-pipelined echo commands internally

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:7
 import threading
+import StringIO
 
----------------
dlj wrote:
> Minor nit: you should usually have access to cStringIO, which might be faster. The common pattern to use it goes like this:
> 
> 
> ```
> try:
>   import cStringIO as StringIO
> except ImportError:
>   import StringIO
> ```
Done. It's kind of a micro-optimization, so I wasn't sure if it was worth it.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:262
+
+    if args:
+        for arg in args[:-1]:
----------------
dlj wrote:
> (Syntax nit) It looks like args will always be a list, even if it's 0-length, right? You can omit the if-check in that case (for-loop over an empty list is effectively a no-op).
I need to check this for `stdout.write(maybeUnescape(args[-1]))`, though. It seemed simpler to put the for loop inside the conditional as well.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:421
+    # temporary file to sidestep blocking pipe write issues.
+    if (cmd.commands[0].args[0] == 'echo' and len(cmd.commands) == 1):
+        output = executeBuiltinEcho(cmd.commands[0], shenv)
----------------
dlj wrote:
> (Minor nit) For consistency, you might want to drop the outer parens. (Or leave them, I just wanted to double-check.)
Oops, this was a two-line conditional that got shortened.


https://reviews.llvm.org/D35093