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

[Authoring] Allow custom overloads #1795

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dongle-the-gadget
Copy link
Contributor

@dongle-the-gadget dongle-the-gadget commented Sep 20, 2024

Closes #1569.
Closes #667.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Is there no unit test we can add for this?

@dongle-the-gadget
Copy link
Contributor Author

dongle-the-gadget commented Sep 21, 2024

I think we would need a projection that doesn't have overloads, which from my understanding is none.

Or possibly an IL inspector? I'm not sure if we have any ATM.

@Sergio0694
Copy link
Member

Mh. I mean, changes seem fine to me, let's just see if @manodasanW thinks we need tests or not 🙂

@dongle-the-gadget
Copy link
Contributor Author

Oh, one more thing. This change doesn't seem to work with CsWinMD (not the source generator itself), probably due to the Windows.winmd file not being imported properly, though CsWinMD is probably low priority at the moment.

@manodasanW
Copy link
Member

I think we would need a projection that doesn't have overloads, which from my understanding is none.

Or possibly an IL inspector? I'm not sure if we have any ATM.

Mh. I mean, changes seem fine to me, let's just see if @manodasanW thinks we need tests or not 🙂

Can't we just add another pair of functions similar to this with a different method name and then use the Overload attribute on them to define the name that should be used for the overload?

@@ -1561,6 +1561,10 @@ private void AddOverloadAttributeForInterfaceMethods(TypeDeclaration interfaceTy
continue;
}

bool alreadyHasOverload = method.TryGetAttributeWithType(Model.Compilation.GetTypeByMetadataName("Windows.Foundation.Metadata.OverloadAttribute"), out AttributeData _);
if (alreadyHasOverload)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should be skipping when there is an attribute or if we should be checking the overloaded method name in the attribute if one is set and then using that below rather than the one we generate. Once you add the test case, we should be able to see whether the overload attribute is ending up in the winmd or not.

@dongle-the-gadget
Copy link
Contributor Author

Can't we just add another pair of functions similar to this with a different method name and then use the Overload attribute on them to define the name that should be used for the overload?

I’m not sure how that would work for the test. Don’t we have to check the WinMD?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants