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

Update tests to new style #963

Merged
merged 4 commits into from
Aug 5, 2017
Merged

Conversation

jagerman
Copy link
Member

This updates the test suite tests to the new style proposed and started in #898. This is still in progress. (I'm adding commits one-by-one for now, but intend to merge them all into one before merging onto master).

(Also note that test_exceptions and test_multiple_inheritance won't be included here—they are covered by PRs #954 and #960, respectively.)

@jagerman jagerman force-pushed the tests-new-style branch 3 times, most recently from 9fc2741 to 7c1ea72 Compare July 26, 2017 19:36
@jagerman jagerman changed the title WIP: Update tests to new style Update tests to new style Jul 26, 2017
@jagerman jagerman force-pushed the tests-new-style branch 5 times, most recently from 3240811 to ca0fa8c Compare July 28, 2017 01:51
@jagerman
Copy link
Member Author

This PR broke the current py::overload_cast with ICEs under both MSVC 2015 and under clang 3.8 on debian stretch.

I spent some time fighting with MSVC to get it to work, and finally settled on e4fb05a, which manages to simplify it, fix the ICEs on MSVC and clang, and makes it C++11-compatible. I came on the really weird trick to making it work accidentally: dropping the defaulted std::false_type argument makes it work. For some bizarre reason it made MSVC choke on the deductions. But having two functions here--one with the std::false_type argument and one without--makes MSVC perfectly happy.

@dean0x7d - do we need the std::false_type argument at all? We could just drop the second function that takes it, but I'm assuming you had a reason for adding the argument in the first place.

@dean0x7d
Copy link
Member

The solution in e4fb05a will not work for overload sets which have a variable number of arguments, e.g. void foo() and void foo(int). That's the main reason why the two stage template parameters are needed (class template parameters + static member template parameters). See the test.cpp file in https://github.com/dean0x7d/overload_cast for the relevant tests.

The std::false_type argument is there to allow programmatic selection of const vs non-const overloads, based on some type trait. E.g. overload_cast<int>(&W::foo, std::is_same<T, U>{}). It's not needed for manual casting, but it's pretty nice for metaprogramming.

@jagerman
Copy link
Member Author

Rats. I think we may have to switch off overload_cast for MSVC 2015 (at least in the test suite). There's nothing inherently unusual about the use in the test code; the slightly different context just seems to trigger an ICE.

@jagerman
Copy link
Member Author

I think I've found a fix (for the original overload_cast implementation) for MSVC 2015: it seems adding an explicit constexpr cast_overload_impl() {} makes it happy. (But changing the {} to = default; makes the ICE happen again).

@jagerman jagerman force-pushed the tests-new-style branch 2 times, most recently from 152bdcc to 8e3e94c Compare August 4, 2017 00:47
The current `py::overload_cast` is hitting some ICEs under both MSVC
2015 and clang 3.8 on debian with the rewritten test suites; adding an
empty constexpr constructor to the `overload_cast_impl` class seems to
avoid the ICE.
This udpates all the remaining tests to the new test suite code and
comment styles started in pybind#898.  For the most part, the test coverage
here is unchanged, with a few minor exceptions as noted below.

- test_constants_and_functions: this adds more overload tests with
  overloads with different number of arguments for more comprehensive
  overload_cast testing.  The test style conversion broke the overload
  tests under MSVC 2015, prompting the additional tests while looking
  for a workaround.

- test_eigen: this dropped the unused functions `get_cm_corners` and
  `get_cm_corners_const`--these same tests were duplicates of the same
  things provided (and used) via ReturnTester methods.

- test_opaque_types: this test had a hidden dependence on ExampleMandA
  which is now fixed by using the global UserType which suffices for the
  relevant test.

- test_methods_and_attributes: this required some additions to UserType
  to make it usable as a replacement for the test's previous SimpleType:
  UserType gained a value mutator, and the `value` property is not
  mutable (it was previously readonly).  Some overload tests were also
  added to better test overload_cast (as described above).

- test_numpy_array: removed the untemplated mutate_data/mutate_data_t:
  the templated versions with an empty parameter pack expand to the same
  thing.

- test_stl: this was already mostly in the new style; this just tweaks
  things a bit, localizing a class, and adding some missing
  `// test_whatever` comments.

- test_virtual_functions: like `test_stl`, this was mostly in the new
  test style already, but needed some `// test_whatever` comments.
  This commit also moves the inherited virtual example code to the end
  of the file, after the main set of tests (since it is less important
  than the other tests, and rather length); it also got renamed to
  `test_inherited_virtuals` (from `test_inheriting_repeat`) because it
  tests both inherited virtual approaches, not just the repeat approach.
@jagerman jagerman merged commit 391c754 into pybind:master Aug 5, 2017
@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 2017
@jagerman jagerman deleted the tests-new-style branch August 14, 2017 20:25
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.

2 participants