fix: avoid infinite recursion in OverloadsFinalizer

[Nerixyz]

Because OverloadsFinalizer recurses into base classes, visited classes have to be added to finalized_ upon entering instead of when leaving.

I added a test where this failed. It's based on sol::call_detail::void_call. I'm not sure if the test can be made shorter.
Since there wasn't a test for base classes, I also included them there.

Sidenote: I think the MRDOCS_CHECK_OR in the for loop should be MRDOCS_CHECK_OR_CONTINUE, but I couldn't think of a good test-case where this is relevant.

[cppalliance-bot]

An automated preview of the documentation is available at https://897.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://897.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

Since there wasn't a test for base classes, I also included them there.

I would put the rest of the test in test-files/golden-tests/config/inherit-base-members/recursion.cpp to keep the conversion of focusing on what features are support (what is being done rather than why) and make the filename self-explanatory.

There are lots of other tests for base classes there. Also, considering the other tests there, is this portion of the test contributing anything to testing what's being fixed in the PR?

struct BaseBase1 {
    void foo(bool);
};

struct BaseBase2 {
    void foo(bool);
};

struct Base1 : public BaseBase1 {
    void foo(int);
};

struct Base2 : private BaseBase2 {
    void foo(double);
};

struct User : public Base1, Base2 {
    void foo(int);
};

I'm not sure if the test can be made shorter.

Yes. The test could be simplified with a more straightforward case that triggers recursion. Another file with this test could ensure that whatever problem is in Sol2 is kept being tested. In any case, this test is also OK.

Sidenote: I think the MRDOCS_CHECK_OR in the for loop should be MRDOCS_CHECK_OR_CONTINUE, but I couldn't think of a good test-case where this is relevant.

Yes. They should be MRDOCS_CHECK_OR_CONTINUE. That's a bug. I caught other similar bugs in for loops of functions that return void.

Another sidenote: this finalized_ name is used in many finalizers but is not the best name since we typically mark it as "finalized" at the start to avoid infinite recursion.

[Nerixyz]

Also, considering the other tests there, is this portion of the test contributing anything to testing what's being fixed in the PR?

No, it's only an additional check that methods from base classes are also considered (and a bit of a sanity check that my condition at the top is correct).

Yes. They should be MRDOCS_CHECK_OR_CONTINUE. That's a bug.

Should I fix this in this PR or open another one?

[cppalliance-bot]

An automated preview of the documentation is available at https://897.mrdocs.prtest2.cppalliance.org/index.html