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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: utils/lit/lit/TestRunner.py:748
     named_temp_files = []
+    builtin_commands = ['cat']
+    builtin_commands_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "builtin_commands")
----------------
I'd make this a set, since that matches the API we're using.


================
Comment at: utils/lit/lit/TestRunner.py:785
         executable = None
+        is_builtin_cmd = False;
         # For paths relative to cwd, use the cwd of the shell environment.
----------------
You can simplify all this code like this:
```
    is_builtin_cmd = args[0] in builtin_cmds
    if not is_builtin_cmd:
        # For paths relative to cwd, use the cwd of the shell environment.
        if args[0].startswith('.'):
...
```

Make all the code that sets executable and diagnoses missing commands only run when we're not dealing with a builtin command.

Leave executable as None, and you won't have to have the ternary below in the Popen call.


================
Comment at: utils/lit/lit/TestRunner.py:810
         args = expand_glob_expressions(args, cmd_shenv.cwd)
+        if is_builtin_cmd == True:
+                args.insert(0, executable)
----------------
There's no need to test bools for equality with other bools. `if is_builtin_cmd:` is simpler, here and below.


================
Comment at: utils/lit/lit/TestRunner.py:811
+        if is_builtin_cmd == True:
+                args.insert(0, executable)
+                args[1] = os.path.join(builtin_commands_dir ,args[1] + ".py")
----------------
This should be indented just 4 spaces


https://reviews.llvm.org/D43501