Skip to content

Removing unnecessary comments #114190

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

Merged
merged 10 commits into from
Apr 5, 2025
Original file line number Diff line number Diff line change
@@ -1,38 +1,18 @@
using System;
namespace System.Reflection.Metadata.ApplyUpdate.Test
{
// This file was autogenerated by copilot.
public static class IncreaseMetadataRowSize
{
public static void Main(string[] args) { }
public static int Method1()
{
return 0;
}

public static void Method2(int x2)
{
// Example body for Method2
// You can implement logic for x2 here if desired.
}

public static void Method3(int x3)
{
// Example body for Method3
// You can implement logic for x3 here if desired.
}

public static void Method4(int x4)
{
// Example body for Method4
// You can implement logic for x4 here if desired.
}

public static void Method5(int x5)
{
// Example body for Method5
// You can implement logic for x5 here if desired.
}
}

}
using System;
namespace System.Reflection.Metadata.ApplyUpdate.Test
{
// This file was autogenerated by copilot.
public static class IncreaseMetadataRowSize
{
public static void Main(string[] args) { }
public static int Method1()
{
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add license headers?

Also, "This file was autogenerated by copilot." is not useful and it should be deleted. If we were adding a comment like that to all pieces of code that AI helped with, we would have them all over the place.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

return 0;
}
public static void Method2(int x2) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static void Method2(int x2) {}
public static void Method2() {}

Do these methods even need the arguments?

Copy link
Member Author

@thaystg thaystg Apr 3, 2025

Choose a reason for hiding this comment

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

Yes, to increase the string heap on metadata I preferred to add a parameter on each method than adding the double number of the methods.

Copy link
Member

@jkotas jkotas Apr 4, 2025

Choose a reason for hiding this comment

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

Ah, ok. I thought that the test is trying to go over some threshold limit related to number of methods (ie methoddef tokens) given that it has very large number of methods.

It sounds like the test is actually trying to get over the 64k limit for the string blob size. Is that right? It would be useful to mention in a comment somewhere. It is not obvious from the test code.

Also, I assume that the number of methods can be much smaller if we just gave them longer names, like void VeryLooooooooooooooooooooooooooooooooongMethodNameToPushTheStringBlobOver64k_1() { }, void VeryLooooooooooooooooooooooooooooooooongMethodNameToPushTheStringBlobOver64k_2() { }, ... - we would only need less than 1000 of those. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I did that now. Thanks!

public static void Method3(int x3) {}
public static void Method4(int x4) {}
public static void Method5(int x5) {}
}

}
Loading