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

Update ICSharpCode.Decompiler to 7.2.1.6856 (#67875) #69065

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

CyrusNajmabadi
Copy link
Member

Revert "Merge pull request #67919 from dibarbet/revert_icsharpcodedecompiler"

This reverts commit 85eaf94, reversing changes made to 0f8a8ed.

…odedecompiler"

This reverts commit 85eaf94, reversing
changes made to 0f8a8ed.
@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners July 17, 2023 14:56
@@ -1198,7 +1198,7 @@ class C
instance void .ctor () cil managed
{
// Method begins at RVA 0x207f
// Code size 7 (0x7)
// Code size: 7 (0x7)
Copy link
Member Author

Choose a reason for hiding this comment

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

new C# decompiler syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the change... I changed this for consistency with other info we print and did not realize in time that ildasm does not use a colon...

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler ptal. @genlu ptal.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler ptal.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-infrastructure ptal.

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Signing off on the infrastructure and IDE portions.

@@ -21,7 +21,7 @@ namespace Microsoft.CodeAnalysis.Editor.UnitTests.MetadataAsSource
[UseExportProvider]
public abstract partial class AbstractMetadataAsSourceTests : IAsyncLifetime
{
protected static readonly string ICSharpCodeDecompilerVersion = "7.1.0.6543";
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but can't this code just read the version number from the file?

{{
return _message;
}}
/*Error: Empty body found. Decompiled assembly might be a reference assembly.*/
Copy link
Member

Choose a reason for hiding this comment

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

This part of the output isn't really the point of the test, but any idea why this might have changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, def odd.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for minor updates to test files based on new decompiler update.


private static string RemoveHeaderComments(string value)
{
return s_headerCommentsRegex.Replace(value, "");
Copy link
Member

Choose a reason for hiding this comment

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

Since we are already doing this, could we also remove the colon from // Code size: 6 (0x6) comments to reduce the diffs?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's nto the worst idea :) But the works already been done. so i'd prefer to keep as is :)

Copy link
Contributor

@RikkiGibson RikkiGibson Jul 26, 2023

Choose a reason for hiding this comment

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

I'm not sure I understand which IL baselines are impacted here. Basically, for a change like this I'd expect many many more baselines to be impacted. Is the issue that we use ildasm in some places and ICSharpCode.Decompiler in other places? I'm concerned about the inconsistency in baselines and about ongoing PRs being disrupted by the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, i'm not sure what a good strategy is here. As long as the compiler is using this tool, and as long as this tool changes their outpus across releases, then we have this problem.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler ptal.

@CyrusNajmabadi
Copy link
Member Author

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler ptal. This fixes a bad memory oom :-)

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) July 26, 2023 23:31
@CyrusNajmabadi CyrusNajmabadi merged commit 3ce621c into dotnet:main Jul 27, 2023
26 of 27 checks passed
@ghost ghost added this to the Next milestone Jul 27, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the revert2 branch July 27, 2023 17:01
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
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.

6 participants