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

[Feature]: Custom ReferenceImporter in MemberCloner #283

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Mar 27, 2022

A draft pull request for addressing #93

One of the major design decisions I made was to require that any custom importers inherit from CloneContextAwareReferenceImporter instead of ReferenceImporter or a hypothetical IReferenceImporter interface. I felt that this was the cleanest way to maintain correct references between cloned members.

@ds5678 ds5678 marked this pull request as draft March 27, 2022 05:54
@ds5678
Copy link
Contributor Author

ds5678 commented Mar 28, 2022

@Washi1337 thoughts?

@Washi1337
Copy link
Owner

Washi1337 commented Mar 28, 2022

Yes, this might be the only way that I can see for now that will ensure proper references will be used, while also allowing consumers to customize the behavior of the importer.

Side note: It may be best to not change existing constructors by adding optional parameters, but rather add constructor overloads instead. Adding optional parameters to a method / constructor is a breaking change on ABI level, forcing consumers to recompile their projects that use this feature.

Introducing breaking changes I think should only happen if we want to bump up a major version (as per semver conventions). I made this mistake once with the introduction of Utf8String, causing quite a bit of trouble among developers updating from 4.5.0 to 4.6.0 :). While I think this is a nice addition, I don't think we need to bump to 5.0.0 just yet with this.

@Washi1337 Washi1337 added enhancement dotnet Issues related to AsmResolver.DotNet labels Mar 28, 2022
@ds5678 ds5678 marked this pull request as ready for review March 28, 2022 17:43
@ds5678
Copy link
Contributor Author

ds5678 commented Mar 28, 2022

Fixed!

@Washi1337 Washi1337 added this to the 4.10.0 milestone Mar 29, 2022
@ds5678 ds5678 changed the title Feature/Custom ReferenceImporter in MemberCloner [Feature]: Custom ReferenceImporter in MemberCloner Mar 29, 2022
Copy link
Owner

@Washi1337 Washi1337 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in response. I have added a few minor questions / comments.

src/AsmResolver.DotNet/Cloning/MemberCloneContext.cs Outdated Show resolved Hide resolved
src/AsmResolver.DotNet/Cloning/MemberCloneContext.cs Outdated Show resolved Hide resolved
@ds5678 ds5678 force-pushed the Feature/CustomReferenceImportersInMemberCloner branch from b8002f5 to b5ac446 Compare March 30, 2022 13:39
@ds5678
Copy link
Contributor Author

ds5678 commented Mar 30, 2022

Should be good

Copy link
Owner

@Washi1337 Washi1337 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@Washi1337 Washi1337 merged commit 415e207 into Washi1337:development Mar 30, 2022
@ds5678 ds5678 deleted the Feature/CustomReferenceImportersInMemberCloner branch March 30, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet Issues related to AsmResolver.DotNet enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants