[PATCH] D92774: [clang][cli] Add command line marshalling tests
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 7 16:03:23 PST 2020
dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.
I think the content here is mostly good, but there a number of things intermingled so it's hard see what's going on. I suggest splitting out one or more NFC commits to land ahead of time.
Besides the main point of the patch, I see at least the following NFC changes:
- Rename of `CC1CommandLineGenerationTest` to `CommandLineTest`.
- (Maybe also rename of `OptsPopulationTest` to `CommandLineTest`?)
- Rename of `CompilerInvocation` variable from `CInvoke` to `Invocation`.
- Rename of second variable from `CInvok1` to `Invocation2`.
- Move of `CompilerInvocation` variable from the individual test to the fixture.
- (Could/should the second variable also be moved to the class?)
- Removal of `"clang", "-xc++", ` from the front of the `Args` vector.
If I were doing this work, I might consider splitting each of those into a separate commit, but I'm not sure where I'd land; the main point is to get the NFC stuff out of the way ahead of time so the interesting content is easy to read (for review, log, blame, etc.).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92774/new/
https://reviews.llvm.org/D92774
More information about the cfe-commits
mailing list