[BUGFIX] Fixing pybind11::error_already_set.matches to also work with exception subclasses

[YannickJadoul]

The error_already_set.matches implementation (introduced in #772) forwards the two exception types in the wrong order as arguments to PyErr_GivenExceptionMatches. This PR fixes that (and renames the parameter/argument from ex to exc to explicitly match the Python documentation).

The reason this bug didn't cause a lot of harm and wasn't notice before is that the problem is only noticeable when matching to an exception base class (otherwise, the order of the arguments does not end up mattering).

This demonstrates the error, as ModuleNotFound error is a subclass of ImportError in Python >= 3.6:

try {
    return py::module::import("nonexistent");
}
catch (py::error_already_set &e) {
    py::print(e.matches(PyExc_ModuleNotFoundError)); // == True
    py::print(e.matches(PyExc_ImportError)); // == False
}

[bstaletic]

Can you make that example that shows the error a test case, so that we have a test that fails without this PR, but works with it?

[YannickJadoul]

Good point; will do!

[YannickJadoul]

@bstaletic I already rebased. Any idea why that the docs fail to build? Something seems wrong with sphinx/doxygen/breathe?

[bstaletic]

Exception occurred:
  File "/home/travis/.local/lib/python3.6/site-packages/breathe/renderer/sphinxrenderer.py", line 47, in parse_definition
    ast = parser.parse_declaration("class", "class")
TypeError: parse_declaration() takes 2 positional arguments but 3 were given

I'm guessing some python library got updated and broke things...

[bstaletic]

Aha! breathe-doc/breathe#411 broke the build because pybind requires "sphinx<2.0", but allows for the latest breathe.

Two options:

• Try with the latest sphinx
• Limit the version of breathe

[YannickJadoul]

Great; thanks for figuring that out. But then it's got nothing to do with this PR, luckily :-)

• Try with the latest sphinx

I think that should work, since .travis.yml currently says # Breathe does not yet support Sphinx 2. Should we (or you, since you figured it out) turn that into a small PR to fix 'master'?

[YannickJadoul]

Done, I think; let's see if all tests now work.

I also had to fix the old test on error_already_set::matches because it never actually entered the catch block where the functionality was tested.

[YannickJadoul]

All green! Thanks for the CI investigation and fix, @bstaletic!

@wjakob, if you!d be thinking about a new patch release, this one might be interesting?

[wjakob]

Nice catch -- merged!