This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Add tuple trivial destructor test
ClosedPublic

Authored by jloser on Sep 5 2021, 4:12 PM.

Details

Summary

There is only compile-time tests in dtor.pass.cpp, so it could be made a
dtor.compile.pass.cpp. Instead, add a runtime test for testing the trivial
destructor behavior for tuple.

Diff Detail

Event Timeline

jloser requested review of this revision.Sep 5 2021, 4:12 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2021, 4:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Sep 6 2021, 11:29 AM

LGTM!

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.compile.pass.cpp
26–31

While you're at it, I would be nice if you can remove the unneeded whitespace in > >.

jloser updated this revision to Diff 370979.Sep 6 2021, 6:30 PM

[NFC] Remove extra whitespace from test

jloser marked an inline comment as done.Sep 6 2021, 6:35 PM
ldionne requested changes to this revision.Sep 7 2021, 9:36 AM
ldionne added a subscriber: ldionne.

Instead of moving this to a .compile.pass.cpp, I'd like to add an actual runtime test that the dtor does the right thing. Something like this:

struct TrackDtor {
  bool* dtorCalled_;
  constexpr explicit TrackDtor(bool* dtorCalled) : dtorCalled_(dtorCalled) { }
  TEST_CONSTEXPR_CXX20 ~TrackDtor() { *dtorCalled_ = true; }
};

TEST_CONSTEXPR_CXX20 bool test() {
  bool called = false;
  {
    std::tuple<TrackDtor> tuple(TrackDtor(&called));
    assert(!called);
  }
  assert(called);

  return true;
}

int main(int, char**) {
  test();
#if TEST_STD_VER > 20
  static_assert(test());
#endif
  return 0;
}
This revision now requires changes to proceed.Sep 7 2021, 9:36 AM
cjdb resigned from this revision.Jan 18 2022, 1:38 PM
jloser updated this revision to Diff 435784.Jun 9 2022, 8:38 PM
jloser retitled this revision from [libc++][NFC] Make tuple dtor test compile test to [libc++][test] Add tuple trivial dtor test.
jloser edited the summary of this revision. (Show Details)

Add runtime test

Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 8:38 PM
jloser edited reviewers, added: var-const, philnik; removed: cjdb, Quuxplusone.Jun 9 2022, 8:40 PM
jloser updated this revision to Diff 435787.Jun 9 2022, 8:46 PM
jloser retitled this revision from [libc++][test] Add tuple trivial dtor test to [libc++][test] Add tuple trivial destructor test.

Mark dtor constexpr in C++20 test

philnik added inline comments.Jun 10 2022, 2:11 AM
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
34

Can this test be constexpr in C++20?

48–52

I think I'd put the static_asserts outside the main.

jloser updated this revision to Diff 436091.Jun 10 2022, 7:15 PM

Move static_asserts out of main
Mark test() function as constexpr in C++20 mode

jloser marked 2 inline comments as done.Jun 10 2022, 7:15 PM
jloser added inline comments.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
34

Yep, just made it so.

48–52

Sure, I don't feel strongly. Moved them outside.

Mordante added inline comments.Jun 11 2022, 11:19 AM
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
40

This differs from @ldionne's suggestion. I wonder whether we should make sure the destruction of the temporary shouldn't increase the counter. Something like:

struct TrackDtor {
   int* count_;
   constexpr explicit TrackDtor(int* count) : count_(count) {}
   constexpr TrackDtor(TrackDtor&& that) : count_(that.count_) { that.count_ = nullptr}
   TEST_CONSTEXPR_CXX20 ~TrackDtor() { if(coount_) ++*count_; }
};

WDYT?

51

Maybe move this to the other static_assert at line 33.

jloser updated this revision to Diff 436156.Jun 11 2022, 11:53 AM
jloser marked 2 inline comments as done.

Modify TrackDtor per Mordante's suggestion to make the intent of the test a bit clearer

jloser marked 2 inline comments as done.Jun 11 2022, 11:54 AM
jloser added inline comments.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
40

I like that idea and find it helps clarify the intent of the test to help future readers.

51

I don't feel strongly; I moved them up above the test() function though and also added a static_assert(test()) while we're at it.

Mordante accepted this revision.Jun 11 2022, 12:00 PM

Thanks for the update! LGTM modulo one nit.

philnik accepted this revision.Jun 11 2022, 12:52 PM

LGTM with tests passing. I think @ldionne's requests have been addressed, so I think you can land this.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
55–56
jloser updated this revision to Diff 436163.Jun 11 2022, 1:02 PM
jloser marked 2 inline comments as done.

Only static_assert(test()) in C++20 or later

jloser marked an inline comment as done.Jun 11 2022, 1:03 PM
jloser added inline comments.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
55–56

I assume you mean > 17 since it's constexpr in C++20. Fixed.

philnik added inline comments.Jun 11 2022, 1:04 PM
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/dtor.pass.cpp
55–56

Oops. Yes I meant > 17.

jloser updated this revision to Diff 436167.Jun 11 2022, 1:47 PM
jloser marked an inline comment as done.

Fix C++11 CI build

This revision was not accepted when it landed; it landed in state Needs Review.Jun 11 2022, 5:12 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.