Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template apply #203

Merged
merged 4 commits into from Mar 20, 2019
Merged

Template apply #203

merged 4 commits into from Mar 20, 2019

Conversation

zhihaoy
Copy link
Contributor

@zhihaoy zhihaoy commented Mar 20, 2019

Description

TEST_CAST_TEMPLATE_INSTANTIATE is nice, but doesn't work for computed type lists. Here we just use predeclared std::tuple as a type list, and add a new macro TEST_CAST_TEMPLATE_APPLY to instantiate those types.

Our need for this feature is strong. It's not only useful in testing generic code, but also useful in testing different implementations for type-erased interface. The list of types with expected semantics in the first case and the list of available classes for implementations varies from build to build on different platforms, etc.

@onqtam
Copy link
Member

onqtam commented Mar 20, 2019

I changed it to be against the dev branch instead of master and resolved some conflicts.

Thanks for the patch! I'm thinking about this right now.

@zhihaoy
Copy link
Contributor Author

zhihaoy commented Mar 20, 2019

Oh, master is not dev... I can do a rebase during the review.

Here is a code snippet to show how we use it:

In library header:

using implementations =
    std::conditional_t<kHaveBLAS, std::tuple<blas, simd>, std::tuple<simd>>;

In tests, which can also be in library code since this is doctest :)

TEST_CASE_TEMPLATE_DEFINE("math op1", T, test_math_op1) {
   ...
   c.Build<T>()
   ...
}

TEST_CASE_TEMPLATE_APPLY(test_math_op1, implementations);

So basically, the current TEST_CASE_TEMPLATE_INSTANTIATE is a hard-coded approach. Our software is macro-free (no code logic decision made using macros), so the existing typed test cases do not help.

@onqtam
Copy link
Member

onqtam commented Mar 20, 2019

So if this gets accepted - the implementation will be relying on a tuple. What other ways are there in modern C++ to express type lists at compile time? I'm just cautious before closing this door and committing to std::tuple.

Also I'll change it to a vararg macro after I merge it so you don't need a typedef (or "using") before the macro call (because of the commas in between the <> of the tuple)

EDIT: Perhaps any sane C++ type list can be translated to a tuple so I need not worry.

@onqtam onqtam merged commit 73ff612 into doctest:dev Mar 20, 2019
@onqtam
Copy link
Member

onqtam commented Mar 20, 2019

@zhihaoy do you think it's possible to make TEST_CASE_TEMPLATE_INSTANTIATE work both with lists and with a tuple so we don't have to introduce a new macro?

Perhaps detecting if what is passed is a tuple, in which case to not wrap it further in a tuple?

Or how would you feel about this name: TEST_CASE_TEMPLATE_INSTANTIATE_TUPLE instead of TEST_CASE_TEMPLATE_APPLY?

EDIT: I'm actually going ahead and renaming it.

onqtam added a commit that referenced this pull request Mar 20, 2019
@zhihaoy
Copy link
Contributor Author

zhihaoy commented Mar 20, 2019

In practice, it is trivial for user to transform their type list into std::tuple https://gist.github.com/mathewmariani/f47056531f9cbbe7be902d8b1410228e#file-elisa-hpp-L61 as long as we accept computed types.

The standard library doesn't define a std::type_list and the committee is hesitate to do so. The thought it that we want a more complete meta programming package, but before that, std::tuple can be used as a compromise. People already have lots of code using it as type list. It's also recommended by several committee members. If you look at this thread https://groups.google.com/a/isocpp.org/forum/m/#!topic/std-proposals/pzK0wxZgAP8 for example, the first reply came from Ville, who is libstdc++ (gcc implementation) maintainer and is also the current Evolution Working Group chair in WG21.

About the naming, if have a chance, I would like to have it changed back to TEST_CASE_TEMPLATE_APPLY. The first reason is of course TEST_CASE_TEMPLATE_INSTANTIATE_TUPLE is too long. A more important reason is that the name "apply" has a much more clear meaning -- the high-order function to take a function and call it with a std::tuple is called std::apply in the standard library. So here, we have a high-order "function" to take a type-parameterized test case and call it with a std::tuple's type, and that high-order "function" (implemented as a macro) is called "APPLY". "Apply" describes what this feature does while the "instantiate" is only an implementation detail.

@zhihaoy
Copy link
Contributor Author

zhihaoy commented Mar 20, 2019

@zhihaoy do you think it's possible to make TEST_CASE_TEMPLATE_INSTANTIATE work both with lists and with a tuple so we don't have to introduce a new macro?

Both are needed. Here is the metaphor I use to explain:
TEST_CASE_TEMPLATE_INSTANTIATE = std::invoke on type-parameterized test cases
TEST_CASE_TEMPLATE_APPLY = std::apply on type-parameterized test cases

@onqtam
Copy link
Member

onqtam commented Mar 20, 2019

Well in that case I would want to rename INSTANTIATE to INVOKE and keep a #define INSTANTIATE INVOKE for backwards-compatibility.

Oh god - why does naming have to be such a time sink :D

EDIT: I'll move back to "APPLY"

onqtam added a commit that referenced this pull request Mar 20, 2019
@zhihaoy zhihaoy deleted the template-apply branch March 21, 2019 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants