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

SerialStream doesn't override memory-based Read-/WriteAsync methods #54575

Closed
ViktorHofer opened this issue Jun 22, 2021 · 5 comments · Fixed by #54544
Closed

SerialStream doesn't override memory-based Read-/WriteAsync methods #54575

ViktorHofer opened this issue Jun 22, 2021 · 5 comments · Fixed by #54544
Assignees
Labels
area-System.IO.Ports help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 22, 2021

When adding a NetCoreAppCurrent configuration to System.IO.Ports, roslyn warns that SerialStream's memory-based ReadAsync and WriteAsync methods aren't overridden. I was unsure if this requires api-approval so I'm filing this issue.

/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.cs(13,35): error CA1844: 'SerialStream' overrides array-based 'ReadAsync' but does not override memory-based 'ReadAsync'. Consider overriding memory-based 'ReadAsync' to improve performance. [/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]
/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.cs(13,35): error CA1844: 'SerialStream' overrides array-based 'WriteAsync' but does not override memory-based 'WriteAsync'. Consider overriding memory-based 'WriteAsync' to improve performance. [/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]

cc @adamsitnik @carlossanlop @jozkee @jeffhandley

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 22, 2021
@ghost
Copy link

ghost commented Jun 22, 2021

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

Issue Details

When adding a NetCoreAppCurrent configuration to System.IO.Ports, roslyn warns that SerialStream's memory-based ReadAsync and WriteAsync methods aren't overridden. I was unsure if this requires api-approval so I'm filing this issue.

/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.cs(13,35): error CA1844: 'SerialStream' overrides array-based 'ReadAsync' but does not override memory-based 'ReadAsync'. Consider overriding memory-based 'ReadAsync' to improve performance. [/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]
/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.cs(13,35): error CA1844: 'SerialStream' overrides array-based 'WriteAsync' but does not override memory-based 'WriteAsync'. Consider overriding memory-based 'WriteAsync' to improve performance. [/home/vihofer/runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]

cc @adamsitnik @carlossanlop @jozkee @jeffhandley

Author: ViktorHofer
Assignees: -
Labels:

area-System.IO

Milestone: -

@stephentoub
Copy link
Member

I was unsure if this requires api-approval so I'm filing this issue.

No approval required.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 23, 2021
@ViktorHofer ViktorHofer self-assigned this Jun 23, 2021
@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jun 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 29, 2021
@stephentoub stephentoub reopened this Jun 30, 2021
@adamsitnik adamsitnik added this to the Future milestone Jun 30, 2021
@adamsitnik
Copy link
Member

@ViktorHofer do you intend to work on it? If not, could please mark it as up-for-grabs and remove your assignment?

@ViktorHofer ViktorHofer removed their assignment Jul 1, 2021
@ViktorHofer ViktorHofer added the help wanted [up-for-grabs] Good issue for external contributors label Jul 1, 2021
i3arnon added a commit to i3arnon/runtime that referenced this issue Aug 4, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 4, 2021
@i3arnon
Copy link
Contributor

i3arnon commented Aug 9, 2021

@adamsitnik Should I take this to mean there's an interest in implementing the ReadAsync & WriteAsync overloads (memory and array based) for the Windows version of SerialStream as well?

@adamsitnik
Copy link
Member

Should I take this to mean there's an interest in implementing the ReadAsync & WriteAsync overloads (memory and array based) for the Windows version of SerialStream as well?

I've reconsidered that and most probably we should not do that for Windows as of now. The main reason is that testing this particular library is really hard and it would be easy to introduce a bug to the product. I am going to close both issues. @i3arnon thanks!

@adamsitnik adamsitnik modified the milestones: Future, 6.0.0 Aug 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Ports help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants