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

[David L. Jones via Phabricator via llvm-commits]

dlj added a comment.

Looks pretty good... just a few nits and one suggestion.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:7
 import threading
+import StringIO
 
----------------
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
```


================
Comment at: llvm/utils/lit/lit/TestRunner.py:245
+    write_newline = True
+    if len(args) >= 1 and args[0].startswith('-'):
+        flag = args[0]
----------------
Would it make sense to use while instead of if? (That would handle the case of 'echo -e -n blah').


================
Comment at: llvm/utils/lit/lit/TestRunner.py:262
+
+    if args:
+        for arg in args[:-1]:
----------------
(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).


================
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)
----------------
(Minor nit) For consistency, you might want to drop the outer parens. (Or leave them, I just wanted to double-check.)


https://reviews.llvm.org/D35093