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

Revert "Typemap cleanup" #1810

Closed

Conversation

deadlocklogic
Copy link
Contributor

Reverts #1802

@deadlocklogic
Copy link
Contributor Author

deadlocklogic commented Dec 16, 2023

@tritao Please read carefully.
While indeed the introduced bug caused unnoticed problems with #1802 (and which I know about now and will resolve separately). I wasn't really aware that we should run CppSharp.Parser.Gen in order to generate bindings for the project itself.
So after reverting this PR, I regenerated the project and run the tests, which failed to compile.
I then reverted all the PR created by myself, and again regenerated and tried to compile, with same error too.
Notice, my other PRs (other than this one trying to revert), had no changes on the diff.
So apparently, some older PR submitted by someone didn't know about running CppSharp.Parser.Gen to synchronize the project's bindings.
Now the good part is, most of the diffs in the generated bindings are just:

  1. removed extra spacing for better syntax
  2. generation of: #pragma warning disable CS9084 // Struct member returns 'this' or other instance members by reference
  3. generation of a routine with callNativeDtor
  4. some __instance are now marked as ref
  5. SourceLocation has now a default constructor

Basically 5 is generating compilation errors:
CS8773: Feature 'parameterless struct constructors' is not available in C# 9.0. Please use language version 10.0 or greater

Summary:

  1. accept GeneratorKind: patch bug caused by missing ToString #1811 which is a simple yet an urgent patch.
  2. accept this PR which will revert Typemap cleanup #1802 which I will rework out, now I aware of project's binding regeneration.
  3. run CppSharp.Parser.Gen
  4. check changes which were introduced by older PR which hasn't generated the project's bindings.

Sorry for the inconvenience. I made a mistake, but looks like others have fallen for the very same one.
The positive side of this mess is that I found out about it before it is too late. Notice your PR #1769 which regenerated the bindings and was created way before my first PR, was failing to compile, maybe it is all related to this same issue.

Edit:

These changes were introduces by many PR before #1783, but #1783 introduced C# 10.0 requirement. And these changes weren't submitted by the corresponding PRs as they should've.

@tritao
Copy link
Collaborator

tritao commented Dec 16, 2023

Should we add a new generator option for the C# language target version? Then we could only enable the behavior of #1783 when its >= 10.0.

Alternatively, we would need to require C# 10.0 going forward.

@deadlocklogic
Copy link
Contributor Author

deadlocklogic commented Dec 16, 2023

Alternatively, we would need to require C# 10.0 going forward.

Indeed this is what I am thinking about. Practically speaking C# 10.0 is really new.
Lets tag @Saalvage and we see if he has an opinion here.

By the way I located the part which needs fixing in this PR, it is solely related to Std.Map typemap, should be a very simple fix.

@tritao
Copy link
Collaborator

tritao commented Dec 16, 2023

By the way, why do we need this revert? Can we not just fix the C# 10.0 issue with the generation?

@deadlocklogic
Copy link
Contributor Author

deadlocklogic commented Dec 16, 2023

As you like, I can close this PR then and submit a patch which fixes it, basically this typemap should be removed as it was in the first place in the CLI backend:

[TypeMap("std::map", GeneratorKindID = GeneratorKind.CSharp_ID)]
public class Map : TypeMap
{
public override bool IsIgnored { get { return true; } }
public override Type SignatureType(TypePrinterContext ctx)
{
if (ctx.Kind == TypePrinterContextKind.Native)
return new CustomType("Std.Map");
var type = Type as TemplateSpecializationType;
return new CustomType(
$@"System.Collections.Generic.Dictionary<{type.Arguments[0].Type}, {type.Arguments[1].Type}>");
}
}

Commenting/removing this will fix the issue.
@tritao Please have a look too.
Logically speaking, why std::map only should have a C# typemap, while std::vector, std::list not? I believe this typemap was inactive while being in the CLI backend while back. But after being refactored to become active in the C# backend, it became undesired.

Notice:

While studying the typemaps in this PR, I noticed that the CLI backend had a dependency on C# and not the opposite. So logically speaking, deleting this typemap won't affect other things.

We should consider adding tests for containers.

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