This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] reworks invocable and regular_invocable tests
ClosedPublic

Authored by cjdb on Mar 25 2021, 8:29 PM.

Details

Summary

The tests for std::invocable and std::regular_invocable were
woefully incomplete. This patch closes many of the gaps (though some
probably remain).

Diff Detail

Event Timeline

cjdb requested review of this revision.Mar 25 2021, 8:29 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 8:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Personally, I don't think these 1000 new lines of code provide enough benefit to offset their cost. The lines-to-useful-tests ratio is very low.

Personally, I don't think these 1000 new lines of code provide enough benefit to offset their cost. The lines-to-useful-tests ratio is very low.

Can you elaborate?

There seems to have been a fixation on line counts lately. If it increases test coverage and does not introduce new duplication, then this change is beneficial.

ldionne accepted this revision.Mar 26 2021, 6:50 AM

This LGTM as far as I'm concerned but I'd like to hear Arthur out.

This revision is now accepted and ready to land.Mar 26 2021, 6:50 AM

Well, it hinges on what you mean by

and does not introduce new duplication

From my POV, obviously this patch introduces MASSIVE duplication. Lines 1-500 are very similar to lines 501-1000. That's literally all. I don't think we should have 1000 lines of highly repetitive tests, just to test a three-line metafunction. It makes it harder for the maintainer to see what's actually tested and what's not.

(Minorly, I think line 258 doesn't match the style we've been using for other constexpr tests. These constexpr functions could all be using assert in the bodies and return true at the end.)

cjdb added a comment.Mar 26 2021, 9:21 AM

Well, it hinges on what you mean by

and does not introduce new duplication

From my POV, obviously this patch introduces MASSIVE duplication. Lines 1-500 are very similar to lines 501-1000. That's literally all. I don't think we should have 1000 lines of highly repetitive tests, just to test a three-line metafunction. It makes it harder for the maintainer to see what's actually tested and what's not.

Each test checks something different, so there aren't any "highly repetitive tests".

The reason the tests are duplicated across two files is because regular_invocable<T> = invocable<T>, and the easiest way to ensure that is to make sure that they pass identical tests. Short of moving everything common into a header and using a macro, there isn't a way to eliminate said duplication across files.

(Minorly, I think line 258 doesn't match the style we've been using for other constexpr tests. These constexpr functions could all be using assert in the bodies and return true at the end.)

It matches the style of all previous concepts tests.

This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp
232

This static assert (and the other one regarding long below on line 237, and two asserts in the other file) fails when building for Windows. On Windows (both 32 and 64 bit), long is the same size as int. Not familiar with why this passes on 32 bit linux then (where long also is the same size as int) but not on Windows - apparently there are differences in how distinct the two types are considered, or something else?

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