This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] rearranges all concept tests
ClosedPublic

Authored by cjdb on Mar 22 2021, 12:42 PM.

Details

Summary

moves tests into directories matching their stable names so that the
tests can reflect the concept name

Diff Detail

Event Timeline

cjdb requested review of this revision.Mar 22 2021, 12:42 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 12:42 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Mar 22 2021, 1:47 PM

I'm fine with this but I would like to see CI passing. Is this based on top of another patch that isn't in main? Also, if we agree that we don't like .compile.pass.cpp tests, I think we should get rid of that test format completely. I must admit I agree with @EricWF's comment about .compile.pass.cpp tests potentially causing runtime tests to be skipped.

This revision is now accepted and ready to land.Mar 22 2021, 1:47 PM

I must admit I agree with @EricWF's comment about .compile.pass.cpp tests potentially causing runtime tests to be skipped.

FWIW, we use a convention that main in a compile-only test is annotated (int main() {} // COMPILE-ONLY) to avoid such problems. This convention predates us actually having technology in our test runners to support compile-only tests, or we'd probably be using the superior int main() { return 1; } // COMPILE-ONLY. (Hmmm, I may go make a PR for that change right now.)

cjdb updated this revision to Diff 332448.Mar 22 2021, 3:22 PM
cjdb edited the summary of this revision. (Show Details)

removed the very unnecessary dependency on D98983

I won't oppose this patch, but FWIW, I would rather keep the .compile.pass.cpp tests and apply a comment as @CaseyCarter suggested. If there's a comment at the end of the function, I don't think anyone will accidentally modify it (and if they do, it will be easy to catch in review). Then we don't have to slow down the test suite (actually, it would be super interesting to see how much slower the test suite is after this change).

libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp
20

What do you think about doing this in two patches?

  1. rename .compile.pass.cpp -> .pass.cpp.
  2. move around the location in tree/rename files.

@zoecarver wrote:

If there's a comment at the end of the function, I don't think anyone will accidentally modify it (and if they do, it will be easy to catch in review).

The failure mode to defend against is "someone creates a new .compile.pass.cpp test (maybe brain fart, or maybe because they think that's just the naming convention), without adding such a comment; and then adds runtime assertions to it." But suppose we have no .compile.pass.cpp tests checked in, so that nobody ever creates a new one by accident (because there's no old ones to copy); then that failure mode goes away forever. Also, any need to explain the difference to newbies also goes away. I think that getting into that world would be worth some (but not an arbitrarily large) buildbot slowdown.

It might be interesting for someone to measure the slowdown from globally changing all .compile.pass.cpp tests into .pass.cpp tests.

cjdb added inline comments.Mar 23 2021, 1:46 PM
libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp
20

What's the motivation? I'm not against it, just want to make sure the effort is justified.

cjdb updated this revision to Diff 333161.Mar 24 2021, 3:48 PM
cjdb retitled this revision from [libcxx] renames and moves all concept tests to [libcxx] rearranges all concept tests.
cjdb edited the summary of this revision. (Show Details)

@zoecarver, for your inspection

The original motivation for .compile.pass.cpp is to avoid running the tests when it's not needed. It doesn't actually matter in any current configuration we test in the CI, but it does matter a bit more when the executor is slow, for example when running the tests through SSH or when running on an emulator for some embedded chip. I still think it would be reasonable to get rid of them for the reasons explained by others, but I wanted to clarify what the actual purpose of that test format is.

Thanks for separating the patches. Now it's super easy to verify correctness. This LGTM (sans the comment about functions.h).

libcxx/test/std/concepts/callable/invocable.compile.pass.cpp
21

Where else is functions.h used? Maybe we should move it into this directory. Or maybe in the support folder?

cjdb added a comment.Mar 26 2021, 10:06 AM

Will merge once CI passes.

libcxx/test/std/concepts/callable/invocable.compile.pass.cpp
21

I deliberately kept this one here because it's pretty specific to the tests for [concepts.callable] (and if I get my way, functions.h will be deleted in D99398).

cjdb updated this revision to Diff 333599.Mar 26 2021, 11:14 AM

rebases to (hopefully) fix CI

This revision was automatically updated to reflect the committed changes.
libcxx/test/std/concepts/concepts.object/copyable.compile.pass.cpp