[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