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

Turn on some CodeAnalysis rules #32837

Merged
merged 3 commits into from
May 20, 2021
Merged

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented May 19, 2021

Contributes to #24055

Going thru https://github.com/dotnet/runtime/blob/main/eng/CodeAnalysis.ruleset and adding the ones that have been enabled in that repo.

@pranavkm pranavkm marked this pull request as ready for review May 19, 2021 13:20
@pranavkm pranavkm requested a review from a team May 19, 2021 13:20
@mkArtakMSFT mkArtakMSFT added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 19, 2021
@pranavkm pranavkm force-pushed the prkrishn/more-codeanalysis-rules branch from 16511e0 to e89535e Compare May 19, 2021 21:15
@pranavkm
Copy link
Contributor Author

🆙 📅

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

I've skimmed through this and can't spot anything to get worried about. All looks good!

One aspect that I'm slightly unsure about is removing unused things that seem to relate to external specifications (e.g., the lists of "libs" in UnsafeNativeMethods.cs, or the things in H2SpecCommands.cs). Arguably there's value in representing the full list to avoid confusion about what the missing values are. However since none of that is code that I have a lot of close expertise with, assuming the code owner is happy then I'm sure it's fine!

@pranavkm
Copy link
Contributor Author

@Tratcher the changes @SteveSandersonMS is talking about are largely in http.sys and IIS's sources. Could you have a look and let me know what your recommendation is?

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

The UnsafeNclNativeMethods cleanup is fine. The h2spec consts I would keep only if it were product code, but this is a test tool that I don't expect to modify.

@pranavkm pranavkm merged commit af9bb41 into main May 20, 2021
@pranavkm pranavkm deleted the prkrishn/more-codeanalysis-rules branch May 20, 2021 16:30
@ghost ghost added this to the 6.0-preview6 milestone May 20, 2021
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants