- User Since
- Sep 27 2019, 5:18 PM (40 w, 2 d)
Thanks for the review, it's a big patch. 😅 I'm a bit busy at the moment, I will respond to the other comments later this week or sometime next week.
Wed, Jul 1
Addressed review comment: replace shows -> show.
Mon, Jun 29
Please review D82791: "[lit] Improve lit's output with default settings and --verbose." before reviewing this revision.
I've split this patch up into four different patches. Here is the suggested review order:
- [docs] [lit] Add a more helpful description for lit.py's -s flag.
- [NFC] [lit] Separate verbose and showOutput.
- [lit] Improve lit's output with default settings and --verbose.
Include missing commit separating verbose and showOutput.
Fri, Jun 19
May 31 2020
On second thought, I'm going to update the documentation alongside the behavior change, to make sure the two are in sync.
Patch 1: Change behavior of -v. (and default and -vv behavior). Update https://llvm.org/docs/CommandGuide/lit.html as necessary.
Patch 2: Add highlighting/colored output. Update https://llvm.org/docs/CommandGuide/lit.html as necessary.
Patch 3: Rename echo_all_commands to runWithCommandOutput.
May 28 2020
Quick question: We show the command output up to and including the failing line. Should we do the same for the script part, i.e., skip the un-executed part of the script? Is it ever useful to know which script line/command would have been executed next?
@JDevlieghere, that is part of the code that is writing out the command line.
Hmm, Phabricator attached my inline comment to my other larger comment, that's confusing.
I'm guessing I was supposed to make an out-of-line comment paired with the inline one,
that's what the "not submitted" was about.
Fixed some variable names, added some doc comments, cleaned up some logic, added some more CHECK lines to tests.
Ah, I just realized there are some inconsistent variable names, some are snake_case, some are camelCase. Should I be consistently using camelCase? I did see a bunch of snake_case variables, that's probably what tripped me up...
Consider the test file:
Mar 25 2020
Closing. I have a PR up against apple/llvm-project. https://github.com/apple/llvm-project/pull/979
Mar 24 2020
Nov 18 2019
Addressed John's suggestion to replace OS X -> macOS.