This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] CompilerInvocationTest: add tests for boolean options
ClosedPublic

Authored by jansvoboda11 on Dec 7 2020, 10:02 AM.

Details

Summary

Add more tests of the command line marshalling infrastructure.

The new tests now make a "round-trip": from arguments, to CompilerInvocation instance to arguments again in a single test case.

The TODOs are resolved in a follow-up patch.

Depends on D92830.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Dec 7 2020, 10:02 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 10:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith requested changes to this revision.Dec 7 2020, 4:03 PM

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.).

This revision now requires changes to proceed.Dec 7 2020, 4:03 PM

Split into multiple patches.

I'm sorry about that, I understand that was hard to review.

This is now split this into multiple patches: D92825, D92826, D92827, D92828, D92829, D92830.

jansvoboda11 retitled this revision from [clang][cli] Add command line marshalling tests to [clang][cli] CompilerInvocationTest: add tests for boolean options.Dec 8 2020, 4:01 AM

WIP, need to fix a typo in the test.

clang/unittests/Frontend/CompilerInvocationTest.cpp
141

This is wrong, let me fix that.

Ready to be reviewed.

This revision is now accepted and ready to land.Dec 8 2020, 6:54 AM
This revision was landed with ongoing or failed builds.Dec 9 2020, 12:00 AM
This revision was automatically updated to reflect the committed changes.

This patch was reverted as it broke a build: http://lab.llvm.org:8011/#/builders/14/builds/2986

The assertions will need to take into account the actual value of LLVM_ENABLE_NEW_PASS_MANAGER.

jansvoboda11 reopened this revision.Dec 9 2020, 1:37 AM
This revision is now accepted and ready to land.Dec 9 2020, 1:37 AM
jansvoboda11 planned changes to this revision.Dec 9 2020, 1:38 AM

Rebase, fix tests and update them to use the new -f[no-]legacy-pass-manager.

This revision is now accepted and ready to land.Dec 11 2020, 5:24 AM

Remove comment

jansvoboda11 requested review of this revision.EditedDec 11 2020, 5:32 AM

@dexonsmith could you look at this again? I've reworked the pass manager tests. They now work with LLVM_ENABLE_NEW_PASS_MANAGER instead of a hard-coded value and use the new -f[no-]legacy-pass-manager flags.

This revision is now accepted and ready to land.Dec 11 2020, 2:14 PM
This revision was landed with ongoing or failed builds.Dec 12 2020, 12:47 AM
This revision was automatically updated to reflect the committed changes.