The tests for std::invocable and std::regular_invocable were
woefully incomplete. This patch closes many of the gaps (though some
probably remain).
Details
- Reviewers
ldionne EricWF miscco Mordante zoecarver • Quuxplusone curdeius - Group Reviewers
Restricted Project - Commits
- rGe06f1a8e3cc6: [libcxx] reworks invocable and regular_invocable tests
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.)
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.
libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp | ||
---|---|---|
232 ↗ | (On Diff #333886) | 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? |