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

Override Stream.ReadByte/WriteByte #33788

Closed
terrajobst opened this issue Mar 19, 2020 · 4 comments
Closed

Override Stream.ReadByte/WriteByte #33788

terrajobst opened this issue Mar 19, 2020 · 4 comments
Labels
area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@terrajobst
Copy link
Member

The rule should flag types derived from Stream that don't override ReadByte or WriteByte.

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Medium

@stephentoub
Copy link
Member

Fixer: Medium

I don't think there's anything meaningful a fixer could do here

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@carlossanlop carlossanlop removed the code-fixer Marks an issue that suggests a Roslyn code fixer label Feb 23, 2021
@carlossanlop
Copy link
Member

This analyzer is related to #33789 . Let's consider adding this case to that one, instead of creating a new analyzer.

And just like in #33789, we should scope this to only types that are directly derived from Stream (and not a derived type).

  • Suggested severity: Info
  • Suggested milestone: Future
  • Suggested category: Performance
  • No fixer: VS already offers the option "Generate overrides", and the user can then select to override ReadByte and WriteByte:
    image

image

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 23, 2021
@bartonjs
Copy link
Member

bartonjs commented Jun 3, 2021

Video

Knowing to override ReadByte and WriteByte is something that would show up in a performance analysis, and a performance-improvements focus would likely have already highlighted it.

Without usage on a specific Stream-derived type saying that ReadByte/WriteByte were even called, this is too low value considering the already high cost of implementing a Stream "properly", and so doesn't seem justified.

The group got sidetracked by theorizing about a DelgatingStream : Stream type and a new intermediate Stream type that rewrites everything in terms of fewer protected abstract methods focused on Span/Memory... but that's not really what this issue was about.

@bartonjs bartonjs closed this as completed Jun 3, 2021
@bartonjs bartonjs removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jun 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

6 participants