[PATCH] D43501: [lit] Implement 'cat' command for internal shell

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Neat!



================
Comment at: utils/lit/lit/TestRunner.py:785
         executable = None
+        isBuiltInCommand = False;
         # For paths relative to cwd, use the cwd of the shell environment.
----------------
Aside from the `kAvoidDevNull` and `kIsWindows` constants, this file mostly uses `foo_bar` underscore style. Please match that style. `is_builtin_cmd` seems consistent.


================
Comment at: utils/lit/lit/TestRunner.py:813
         if kIsWindows:
             args = quote_windows_command(args)
 
----------------
You need to move the logic below that treats args as a list above this call to `quote_windows_command`, which flattens it into an appropriately quoted string on Windows.


================
Comment at: utils/lit/lit/TestRunner.py:816
         try:
-            procs.append(subprocess.Popen(args, cwd=cmd_shenv.cwd,
-                                          executable = executable,
+            tempArgs = None;
+            if isBuiltInCommand == True:
----------------
You don't need `tempArgs` and `tempExecutable`.


================
Comment at: utils/lit/lit/TestRunner.py:817
+            tempArgs = None;
+            if isBuiltInCommand == True:
+                tempArgs = list(args)
----------------
Move this logic out of the try / except.


================
Comment at: utils/lit/lit/builtin_commands/cat.py:3
+
+filenames = sys.argv[1:]
+
----------------
Please wrap this code in `def main():` and use the `if __name__ == '__main__': main()` pattern. I know it's boiler-plate, but running Python code at global scope has gotchas and these things tend to grow and get copy-pasted.


https://reviews.llvm.org/D43501