No 'EntryPoint' attribute is generated for C++ mangled functions

[zeule]

Given the following header:

namespace N {
   int f();
}

the generator fails to add mangled name to the DllImport attribute. Don't really know what am I doing, but the following change seems to make it work:

diff --git a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs
index 097e5df..97bac12 100644
--- a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs
+++ b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs
@@ -464,10 +464,10 @@ namespace ClangSharp
             }
 
             var callingConventionName = GetCallingConvention(functionDecl, callConv, name);
-            var entryPoint = !isVirtual && body is null
-                ? (cxxMethodDecl is null) ? GetCursorName(functionDecl) : cxxMethodDecl.Handle.Mangling.CString
+            var isDllImport = !isVirtual && body is null;
+            var entryPoint = isDllImport
+                ? (cxxMethodDecl is null) ? functionDecl.Handle.Mangling.CString : cxxMethodDecl.Handle.Mangling.CString
                 : null;
-            var isDllImport = body is null && !isVirtual;
 
             var desc = new FunctionOrDelegateDesc<(string Name, PInvokeGenerator This)>
             {

[tannergooding]

This looks correct. Namespace level declarations don't have a lot of coverage today.

[zeule]

Do you mean the current code is correct? Namespace is just a single example, the mangled name in the EntryPoint is required for any function not in extern "C" block.

[tannergooding]

I meant your suggested fix is correct. This was missed because there is not significant coverage for many C++ concepts, one of which is namespace level function declarations.

Most users tend to bind over C libraries, not C++ ones, because there are many potential ABI problems and incompatibilities that can come out for C++.

[zeule]

Thanks for clarifying! Can it be simplified to:

var entryPoint = isDllImport ? functionDecl.Handle.Mangling.CString : null;

?

[tannergooding]

Yes, I believe it can, provided all the existing tests pass anyways

[zeule]

The tests will fail, I guess, because the tool defaults to C++, as per ClangSharpPInvokeGenerator help output, but tests assume C source code if they pass without this change now :)

BTW, could you bump .NET versions for tests? I can't run them on Linux because all I have here is .NET Core 5, and running dotnet test results in

Testhost process exited with error: It was not possible to find any compatible framework version
The framework 'Microsoft.NETCore.App', version '3.1.0' was not found.                                                                                                                                               
  - The following frameworks were found:                                                                                                                                                                            
      5.0.7 at [/opt/dotnet_core/shared/Microsoft.NETCore.App]

[zeule]

BTW, could you bump .NET versions for tests? I can't run them on Linux because all I have here is .NET Core 5, and running dotnet test results in

Never mind, I was stupid, dotnet test accepts --framework option. But now I can't run tests because of #246.

[zeule]

@tannergooding, am I right that the proper fix requires:

1. This change.
2. Teach tests to select input language for code fragments and set it to "C" for some.
3. Change some "expected" text fragments to include the "EntryPoint" property.
4. Teach libClangSharp to select mangling scheme to run Windows tests on Linux and vise-versa
5. Set mangling scheme for tests

Or is there a simpler way?