[PATCH] D44598: [lit] Report line number for failed RUN command

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: utils/lit/lit/TestRunner.py:792
 
+    if cmd.commands[0].args[0] == ':':
+        if len(cmd.commands) != 1:
----------------
delcypher wrote:
> jdenny wrote:
> > delcypher wrote:
> > > The null command is pretty esoteric but occasionally useful in normal shell script. What's the reason it's not supported?
> > > I guess we don't normally need it and it's a waste of time to implement it for the internal shell?
> > That would be my guess.  Do you object to implementing it for this purpose?
> I'm fine with you not implementing the null command. I was just curious. You could perhaps leave a `TODO` or `FIXME` commenting that we don't need to support it right now.
It seems we're miscommunicating.  This does implement the null command.  It just rejects the null command in a pipeline because I don't see a use for that right now.


================
Comment at: utils/lit/lit/TestRunner.py:1367
                 output = []
+            line = ': \'' + keyword + ' at line ' + str(line_number) + \
+                   '\'; ' + line
----------------
delcypher wrote:
> jdenny wrote:
> > delcypher wrote:
> > > I think this would be more readable as
> > > 
> > > ```
> > > line = ": '{keyword} at line {line}'; {real_command}".format(
> > >   keyword=keyword,
> > >   line=line,
> > >   real_command=line)
> > > ```
> > > 
> > > Or something similar. There's no reason for the `\'` if you use `"` quotes instead.
> > Sure, I'll change that and the other occurrence you pointed out.
> Great. I also much prefer use of python's string format function as in the example I gave but I'll leave that up to you.
Yeah, that looks nice too.


https://reviews.llvm.org/D44598