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

Create a non-generic Il2CppArrayBase class #126

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

ds5678
Copy link
Collaborator

@ds5678 ds5678 commented May 13, 2024

This will be important for unstripping ldlen.

@ds5678
Copy link
Collaborator Author

ds5678 commented May 16, 2024

Should Il2CppArrayBase<T>.WrapNativeGenericArrayPointer(IntPtr) become Il2CppArrayBase.WrapNativeGenericArrayPointer<T>(IntPtr)?

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1000

@ds5678
Copy link
Collaborator Author

ds5678 commented May 16, 2024

Should Il2CppArrayBase<T>.WrapNativeGenericArrayPointer(IntPtr) become Il2CppArrayBase.WrapNativeGenericArrayPointer<T>(IntPtr)?

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1000

Now that I think about it some more, this should be a future pull request because it has implications on the generator, which I significantly changed in #124.

Copy link
Member

@ManlyMarco ManlyMarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Il2CppArrayBase or are classes inheriting it directly accessed by any plugin code? If yes, this looks like a binary compatibility break.

@ds5678
Copy link
Collaborator Author

ds5678 commented May 17, 2024

Is Il2CppArrayBase or are classes inheriting it directly accessed by any plugin code? If yes, this looks like a binary compatibility break.

Plugins can and do access these classes.

My pull request has two breaking changes:

  • Moving Length to the base class
    • This is binary breaking but not source breaking.
    • The binary break can be avoided by adding a derived property that hides the base property.
  • Changing IsReadOnly to an explicit interface implementation
    • This is a source and binary breaking change.
    • There is no reason for code to access this. It should have been this way from the start, but I can change it back if desired.

@ds5678
Copy link
Collaborator Author

ds5678 commented May 17, 2024

I'm not sure how much breaking changes matter because my ultimate goal is to make lots of breaking changes. I want to do a proper C# type system with real interfaces and virtual methods. Porting to AsmResolver is just a first step towards that goal.

@ds5678 ds5678 force-pushed the nongeneric-il2cpparraybase branch from db4f65d to 59e4614 Compare May 18, 2024 19:32
@ds5678
Copy link
Collaborator Author

ds5678 commented Jun 29, 2024

The binary break can be avoided by adding a derived property that hides the base property.

Done

@ds5678 ds5678 force-pushed the nongeneric-il2cpparraybase branch from 4e04eb2 to 1262b53 Compare June 29, 2024 16:11
@ManlyMarco ManlyMarco merged commit d3d7ea4 into BepInEx:master Jun 29, 2024
2 checks passed
@ds5678 ds5678 deleted the nongeneric-il2cpparraybase branch June 29, 2024 19:01
@ds5678 ds5678 added this to the 1.4.6 milestone Jul 15, 2024
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.

3 participants