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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
