[PATCH] D43501: [lit] Implement 'cat' command for internal shell
Reid Kleckner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 28 14:18:30 PST 2018
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
More information about the llvm-commits
mailing list