Use ImmutableArray<string> instead of string for arguments#45222
Use ImmutableArray<string> instead of string for arguments#452224 commits merged intodotnet:masterfrom
Conversation
src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContext.cs
Outdated
Show resolved
Hide resolved
...orkspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStreamStorageExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectOptionsProcessor.cs
Outdated
Show resolved
Hide resolved
|
@davkean High level approach looks good; commented on a few places where a few bugs popped out but didn't do a careful review. |
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectOptionsProcessor.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectOptionsProcessor.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectOptionsProcessor.cs
Outdated
Show resolved
Hide resolved
...orkspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStreamStorageExtensions.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs
Outdated
Show resolved
Hide resolved
|
I have no issue with this. |
|
@davkean I've marked this as draft so it's not on our review list, once you're ready for us to take this just let us know. |
|
@jasonmalinowski @CyrusNajmabadi This is ready for re-review. I've tested this, including the ruleset changing on disk path and it works great. |
a5f05e9 to
cbab02d
Compare
Fixes: dotnet#32594. This converts from a flat string to represent the command-line to an ImmutableArray<string> to avoid the string ending up on the LOH.
| using Roslyn.Test.Utilities; | ||
| using Xunit; | ||
|
|
||
| #pragma warning disable CS0618 // Testing obsolete overload |
There was a problem hiding this comment.
Can we move the tests to the non-obsolete overload?
There was a problem hiding this comment.
That would miss coverage.
There was a problem hiding this comment.
I guess if we want to cover the obsolete methods we probably should do so more intentionally, but I assume we're ripping this out as soon as you switch to it...
There was a problem hiding this comment.
Legacy will always call this method.
There was a problem hiding this comment.
At least by name these are the CPS tests though -- should we get a copy of the tests then testing through the old interface too?
src/Workspaces/Core/Portable/Workspace/Solution/Checksum_Factory.cs
Outdated
Show resolved
Hide resolved
|
@davkean Is this waiting on anything else at this point other than the test coverage concern? |
|
I've changed the tests methods to call through the new API. |
Fixes: #32594.
This converts from a flat string to represent the command-line to an ImmutableArray to avoid the string ending up on the LOH.