fix GetFunctionAddress resolution with explicit name change

[conrade-ctc]

Description

We have stricter compiler/flags in our env, and resolution fails due to two GetFunctionAddress definitions. This PR moves the internal function to a different name GetFunctionAddressFromMangledName to fix the resolution.

Type of change

Please tick all options which are relevant.

• [ x ] Bug fix
• New feature
• Requires documentation updates

Testing

In our env, stricter flag for compiler now resolve the issue, no tests added to CppInterOp, just a rename on the surface.

Checklist

• [ x ] I have read the contribution guide recently

[conrade-ctc]

@vgvassilev @aaronj0 for review, viz

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.40%. Comparing base (af760d0) to head (b261dd9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #793   +/-   ##
=======================================
  Coverage   79.40%   79.40%           
=======================================
  Files          11       11           
  Lines        3987     3987           
=======================================
  Hits         3166     3166           
  Misses        821      821           

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	95.12% <ø> (ø)
include/CppInterOp/Dispatch.h	77.77% <ø> (ø)
lib/CppInterOp/CppInterOp.cpp	87.80% <100.00%> (ø)

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	95.12% <ø> (ø)
include/CppInterOp/Dispatch.h	77.77% <ø> (ø)
lib/CppInterOp/CppInterOp.cpp	87.80% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[aaronj0]

Nice, I agree with this change, especially since the overloads for const char* and FunctionDecl were used somewhat differently in the context of testing. It's also good that we no longer need to spell out the fnptr type in the Dispatch, and will be able to provide both versions.

This would be a breaking change in the public API, so maybe we want to visit its external usage in xeus-cpp. cppyy does not rely on the const char* overload, so this shouldn't break anything there.

[aaronj0]

LGTM! Would be good if you could run clang-format before we land this

[vgvassilev]

Are you using the Dispatch.h infrastructure? Otherwise why this particular overload does not work for you?

[conrade-ctc]

@aaronj0, git-clang-format run, looks like a pre-commit hook isn't properly set up on our side, so formatting isn't automated, sorry about that.

BTW, does your process do a squash of the commit, or do you typically force push after an amend?

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[aaronj0]

@aaronj0, git-clang-format run, looks like a pre-commit hook isn't properly set up on our side, so it formatting isn't automated, sorry about that.

BTW, does your process do a squash of the commit, or do you typically force push after an amend?

No worries, thanks! We usually land PRs either squashed into a single commit or rebased with atomic commits. amend + force-push is also welcome if you prefer a cleaner history (atleast I do :)). I think the only policy is that we don't use merge commits.

[conrade-ctc]

Are you using the Dispatch.h infrastructure? Otherwise why this particular overload does not work for you?

Just running your test suite causes this on our side, but we are compiling with -std=c++20, not -std=c++17, and because of that have a number of different compiler flags (partially because of our build setup which has e.g. -Werror etc, so is stricter). I think (but didn't dig deeply) that it generally results from this change to compiling everything with -std=c++20, which causes more failures in your normal build/test cycle (on our side, but using your documented build instructions with this one change to CMakeLists.txt). I just tried to reproduce the specific error we were getting on our side but in your build setup, but was unable to, will try to pinpoint it in your build/test setup as mentioned, there a bunch of other tests failing when I start the process of moving the normal CppInterOp toolchain towards the one we're using.

This was the specific error: error: cannot initialize a parameter of type 'CppImpl::TCppFunction_t' (aka 'void *') with an lvalue of type 'const char[9]', and that was for the invocations in the test using GetFunctionAddress("ret_zero").

[conrade-ctc]

@aaronj0, git-clang-format run, looks like a pre-commit hook isn't properly set up on our side, so it formatting isn't automated, sorry about that.
BTW, does your process do a squash of the commit, or do you typically force push after an amend?

No worries, thanks! We usually land PRs either squashed into a single commit or rebased with atomic commits. amend + force-push is also welcome if you prefer a cleaner history (atleast I do :)). I think the only policy is that we don't use merge commits.

I'll force push a single squashed commit then (assuming you don't have the github policy set to have that as the default behavior, which it sounds like you don't!). Thanks.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

Are you using the Dispatch.h infrastructure? Otherwise why this particular overload does not work for you?

Just running your test suite causes this on our side, but we are compiling with -std=c++20, not -std=c++17, and because of that have a number of different compiler flags (partially because of our build setup which has e.g. -Werror etc, so is stricter). I think (but didn't dig deeply) that it generally results from this change to compiling everything with -std=c++20, which causes more failures in your normal build/test cycle (on our side, but using your documented build instructions with this one change to CMakeLists.txt). I just tried to reproduce the specific error we were getting on our side but in your build setup, but was unable to, will try to pinpoint it in your build/test setup as mentioned, there a bunch of other tests failing when I start the process of moving the normal CppInterOp toolchain towards the one we're using.

This was the specific error: error: cannot initialize a parameter of type 'CppImpl::TCppFunction_t' (aka 'void *') with an lvalue of type 'const char[9]', and that was for the invocations in the test using GetFunctionAddress("ret_zero").

I really want to dig into this because it looks like it tried to make an array to pointer decay of a string literal and select the void* overload instead of the const char* one.

[conrade-ctc]

Are you using the Dispatch.h infrastructure? Otherwise why this particular overload does not work for you?

Just running your test suite causes this on our side, but we are compiling with -std=c++20, not -std=c++17, and because of that have a number of different compiler flags (partially because of our build setup which has e.g. -Werror etc, so is stricter). I think (but didn't dig deeply) that it generally results from this change to compiling everything with -std=c++20, which causes more failures in your normal build/test cycle (on our side, but using your documented build instructions with this one change to CMakeLists.txt). I just tried to reproduce the specific error we were getting on our side but in your build setup, but was unable to, will try to pinpoint it in your build/test setup as mentioned, there a bunch of other tests failing when I start the process of moving the normal CppInterOp toolchain towards the one we're using.
This was the specific error: error: cannot initialize a parameter of type 'CppImpl::TCppFunction_t' (aka 'void *') with an lvalue of type 'const char[9]', and that was for the invocations in the test using GetFunctionAddress("ret_zero").

I really want to dig into this because it looks like it tried to make an array to pointer decay of a string literal and select the void* overload instead of the const char* one.

@vgvassilev, so it turns out that this is ONLY due to -DENABLE_DISPATCH_TESTS for the DynamicLibraryManagerTests, so it was pulling in the dispatching to that test... your base doesn't include that def for that separate test, probably intentionally, but that was the source of the incorrect resolution.

[aaronj0]

@vgvassilev, so it turns out that this is ONLY due to -DENABLE_DISPATCH_TESTS for the DynamicLibraryManagerTests, so it was pulling in the dispatching to that test... your base doesn't include that def for that separate test, probably intentionally, but that was the source of the incorrect resolution.

Indeed, I suspected that the only possibility is if DynamicLibraryManagerTests was running with dispatch. That exclusion was intentional because the overload wasn't supported. Something we should work on once some bandwidth frees up..

I would perhaps hold off on merging this since a rename is a breaking change, and assuming this patch isn't a blocker for anything else