Removing unnecessary comments #114190

thaystg · 2025-04-03T00:44:38Z

Addressing @jkotas comment in this other PR: #113995

Copilot

Pull Request Overview

This PR removes extraneous commentary from method implementations in the IncreaseMetadataRowSize test file based on previous feedback.

Removed placeholder comments from Method2, Method3, Method4, and Method5
Simplified each method to an empty implementation

jkotas · 2025-04-03T00:47:27Z

Looks like this has another whitespace conflict?

jkotas · 2025-04-03T02:11:21Z

...stem.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize.cs

+        {
+            return 0;
+        }
+        public static void Method2(int x2) {}


Suggested change

public static void Method2(int x2) {}

public static void Method2() {}

Do these methods even need the arguments?

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.

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?

Right, I did that now. Thanks!

thaystg · 2025-04-03T13:48:00Z

Looks like this has another whitespace conflict?

Fixed.

am11 · 2025-04-03T14:03:50Z

Fixed.

_v1 still has the incorrect line endings. You probably want to set core.autocrlf to true on your machine: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_autocrlf. On my windows box (standard install VS and git-for-windows):

# global autocrlf
$ git config --global core.autocrlf
# is empty

# system autocrlf
$ git config --system core.autocrlf
# is true

If you want to match this config, then from admin shell: git config --system core.autocrlf true. Otherwise, you can set it globally for your user profile git config --global core.autocrlf true without admin command prompt or powershell.

thaystg · 2025-04-03T15:45:12Z

Fixed.

_v1 still has the incorrect line endings. You probably want to set core.autocrlf to true on your machine: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_autocrlf. On my windows box (standard install VS and git-for-windows):
# global autocrlf
$ git config --global core.autocrlf
# is empty

# system autocrlf
$ git config --system core.autocrlf
# is true
If you want to match this config, then from admin shell: git config --system core.autocrlf true. Otherwise, you can set it globally for your user profile git config --global core.autocrlf true without admin command prompt or powershell.

I have already done it, but how did you see that it's wrong yet, for me I see this output when I call:
git ls-files --eol

i/lf    w/crlf  attr/text             	src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize.cs
i/lf    w/crlf  attr/text             	src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize_v1.cs
i/lf    w/crlf  attr/text             	src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize.csproj
i/lf    w/crlf  attr/text=auto        	src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/deltascript.json

Also looked at the file and I see:

am11 · 2025-04-03T16:07:48Z

Ah, I was actually inspecting the patch link https://patch-diff.githubusercontent.com/raw/dotnet/runtime/pull/114190.patch, it shows diffs commit-wise rather than composite diff. Your latest commit indeed fixed the issue. 👍

dotnet-policy-service · 2025-04-03T16:22:33Z

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

noahfalk · 2025-04-03T19:38:10Z

[like] Noah Falk reacted to your message:

…

________________________________ From: Thays Grazia ***@***.***> Sent: Thursday, April 3, 2025 3:45:34 PM To: dotnet/runtime ***@***.***> Cc: Noah Falk ***@***.***>; Review requested ***@***.***> Subject: Re: [dotnet/runtime] Removing unnecessary comments (PR #114190) Fixed. _v1 still has the incorrect line endings. You probably want to set core.autocrlf to true on your machine: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_autocrlf. On my windows box (standard install VS and git-for-windows): # global autocrlf $ git config --global core.autocrlf # is empty # system autocrlf $ git config --system core.autocrlf # is true If you want to match this config, then from admin shell: git config --system core.autocrlf true. Otherwise, you can set it globally for your user profile git config --global core.autocrlf true without admin command prompt or powershell. I have already done it, but how did you see that it's wrong yet, for me I see this output when I call: git ls-files --eol i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize.cs i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize_v1.cs i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize.csproj i/lf w/crlf attr/text=auto src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/deltascript.json — Reply to this email directly, view it on GitHub<#114190 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABPULQBYSK7LYKZEXGLUS3D2XVJR5AVCNFSM6AAAAAB2K4EHDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZWGIZTINRRGM>. You are receiving this because your review was requested.Message ID: ***@***.***> [thaystg]thaystg left a comment (dotnet/runtime#114190)<#114190 (comment)> Fixed. _v1 still has the incorrect line endings. You probably want to set core.autocrlf to true on your machine: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_autocrlf. On my windows box (standard install VS and git-for-windows): # global autocrlf $ git config --global core.autocrlf # is empty # system autocrlf $ git config --system core.autocrlf # is true If you want to match this config, then from admin shell: git config --system core.autocrlf true. Otherwise, you can set it globally for your user profile git config --global core.autocrlf true without admin command prompt or powershell. I have already done it, but how did you see that it's wrong yet, for me I see this output when I call: git ls-files --eol i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize.cs i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize_v1.cs i/lf w/crlf attr/text src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize.csproj i/lf w/crlf attr/text=auto src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/deltascript.json — Reply to this email directly, view it on GitHub<#114190 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABPULQBYSK7LYKZEXGLUS3D2XVJR5AVCNFSM6AAAAAB2K4EHDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZWGIZTINRRGM>. You are receiving this because your review was requested.Message ID: ***@***.***>

jkotas · 2025-04-04T14:15:24Z

...stem.Reflection.Metadata.ApplyUpdate.Test.IncreaseMetadataRowSize/IncreaseMetadataRowSize.cs

@@ -9,30 +9,10 @@ public static int Method1()
        {


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.

jkotas

Thank you!

Removing unnecessary comments

3d35d8f

thaystg requested review from jkotas and Copilot April 3, 2025 00:44

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 3, 2025

Copilot AI reviewed Apr 3, 2025

View reviewed changes

dotnet-policy-service bot assigned thaystg Apr 3, 2025

thaystg requested a review from a team April 3, 2025 00:46

jkotas reviewed Apr 3, 2025

View reviewed changes

thaystg and others added 3 commits April 3, 2025 10:37

removing content

43b6752

Merge branch 'main' into dev/thays/remove_useless_comment_code

800ab08

Removing content of files.

0f2aa94

FIxing newline and adding the content again.

16d661f

tommcdon added area-System.Reflection.Metadata and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 3, 2025

tommcdon added this to the 10.0.0 milestone Apr 3, 2025

build-analysis bot mentioned this pull request Apr 3, 2025

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

tommcdon approved these changes Apr 4, 2025

View reviewed changes

Merge branch 'main' into dev/thays/remove_useless_comment_code

61fa47b

jkotas reviewed Apr 4, 2025

View reviewed changes

Addressing Jank comments

07f783c

Reducing more the number of the methods

4293246

jkotas approved these changes Apr 4, 2025

View reviewed changes

Fix build error

057cd57

build-analysis bot mentioned this pull request Apr 4, 2025

[wasm] at System.Net.Http.BrowserHttpInterop.CancellationHelper #112533

Open

Merge branch 'main' into dev/thays/remove_useless_comment_code

951b828

build-analysis bot mentioned this pull request Apr 5, 2025

SIGKILL System.Runtime.Serialization.Formatters.Tests #114287

Open

jkotas merged commit e2ec536 into dotnet:main Apr 5, 2025
81 of 87 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing unnecessary comments #114190

Removing unnecessary comments #114190

thaystg commented Apr 3, 2025 •

edited

Loading

Copilot AI left a comment

jkotas commented Apr 3, 2025

jkotas Apr 3, 2025

thaystg Apr 3, 2025 •

edited

Loading

jkotas Apr 4, 2025 •

edited

Loading

thaystg Apr 4, 2025

thaystg commented Apr 3, 2025

am11 commented Apr 3, 2025

thaystg commented Apr 3, 2025 •

edited

Loading

am11 commented Apr 3, 2025

dotnet-policy-service bot commented Apr 3, 2025

noahfalk commented Apr 3, 2025 via email

jkotas Apr 4, 2025

thaystg Apr 4, 2025

jkotas left a comment

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

Removing unnecessary comments #114190

Removing unnecessary comments #114190

Conversation

thaystg commented Apr 3, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

jkotas commented Apr 3, 2025

jkotas Apr 3, 2025

Choose a reason for hiding this comment

thaystg Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

jkotas Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

thaystg Apr 4, 2025

Choose a reason for hiding this comment

thaystg commented Apr 3, 2025

am11 commented Apr 3, 2025

thaystg commented Apr 3, 2025 • edited Loading

am11 commented Apr 3, 2025

dotnet-policy-service bot commented Apr 3, 2025

noahfalk commented Apr 3, 2025 via email

jkotas Apr 4, 2025

Choose a reason for hiding this comment

thaystg Apr 4, 2025

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

thaystg commented Apr 3, 2025 •

edited

Loading

thaystg Apr 3, 2025 •

edited

Loading

jkotas Apr 4, 2025 •

edited

Loading

thaystg commented Apr 3, 2025 •

edited

Loading