This is an archive of the discontinued LLVM Phabricator instance.

Use a test fixture for LoopInfoTest.
ClosedPublic

Authored by trentxintong on Jan 16 2017, 12:08 PM.

Details

Summary

I am about to add some new tests to the file in a later patch, but all the
tests in the file have to all be using TEST_F or none.

So i am changing this test proactively for later commit.

Plus the later patch will use the same fixture.

Diff Detail

Repository
rL LLVM

Event Timeline

trentxintong retitled this revision from to Use a test fixture for LoopInfoTest..
trentxintong updated this object.
trentxintong added reviewers: sanjoy, hfinkel.
trentxintong added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Jan 16 2017, 10:20 PM
sanjoy added inline comments.
unittests/Analysis/LoopInfoTest.cpp
27 ↗(On Diff #84582)

Not sure what you're getting out of making this a fixture. Why can't DT adn LI live as locals within runWithLoopInfo?

This revision now requires changes to proceed.Jan 16 2017, 10:20 PM
trentxintong edited edge metadata.

Address comments.

sanjoy requested changes to this revision.Jan 17 2017, 10:18 AM
sanjoy added inline comments.
unittests/Analysis/LoopInfoTest.cpp
26 ↗(On Diff #84695)

Why unique_ptr? Why not just:

DominatorTree DT(F);
LoopInfo LI(DT);

?

Also: why do you need the fixture at all? Why not just have runWithLoopInfo be a (static) free function.

This revision now requires changes to proceed.Jan 17 2017, 10:18 AM
trentxintong edited edge metadata.

We do not actually need a test fixture for this.

sanjoy accepted this revision.Jan 17 2017, 11:57 AM

LGTM

unittests/Analysis/LoopInfoTest.cpp
21 ↗(On Diff #84715)

Please clang-format.

This revision is now accepted and ready to land.Jan 17 2017, 11:57 AM

Running clang-format.

This revision was automatically updated to reflect the committed changes.