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

Disable generated code analysis for CA-1416 #4846

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Feb 13, 2021

As discussed with @terrajobst and @jeffhandley disabling it in 5.0. We will make it configurable by the user within 6.0 when AnalysisContext.AnalyzerOptions.AnalyzerConfigOptionsProvider.GlobalOptions became available in the repo

Copy link
Member

@terrajobst terrajobst left a comment

Choose a reason for hiding this comment

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

This makes sense to me. @mavasani @sharwell, any thoughts?

@sharwell
Copy link
Member

This seems like a valid warning to report in generated code. Is there a reason we need to disable it?

@codecov
Copy link

codecov bot commented Feb 13, 2021

Codecov Report

Merging #4846 (bdb609b) into release/5.0.2xx (ed46154) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           release/5.0.2xx    #4846   +/-   ##
================================================
  Coverage            95.81%   95.82%           
================================================
  Files                 1164     1164           
  Lines               264699   264699           
  Branches             15987    15987           
================================================
+ Hits                253621   253644   +23     
+ Misses                9049     9031   -18     
+ Partials              2029     2024    -5     

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Feb 13, 2021

This seems like a valid warning to report in generated code. Is there a reason we need to disable it?

Yes, but this might not necessary in most cases, especially for users having huge generated files this could cause slowness in the build, we want to make it off by default and user-configurable instead

@Youssef1313
Copy link
Member

I agree with @sharwell here.

  • Warnings are valid and can be helpful for generated code.
  • This breaks developers' experience who are already aware this is reported for generated code.

Have this shown any clear performance impact in practice?

@sharwell
Copy link
Member

@Youssef1313 We had a case where this analyzer was suspected to be causing a performance problem (which led to investigating it), but measurement later ended up showing that this analyzer wasn't even registering as taking any significant time at all.

@mavasani
Copy link
Contributor

@sharwell This PR is unrelated to the performance issue you are citing. I have clarified with @buyaa-n offline that the PR is purely driven by a functional design decision that this diagnostic is unlikely to be useful for any customers in generated code. This aspect will be made configurable in 6.0 branch for customers who want to see these diagnostics in generated code.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Feb 15, 2021

@sharwell This PR is unrelated to the performance issue you are citing. I have clarified with @buyaa-n offline that the PR is purely driven by a functional design decision that this diagnostic is unlikely to be useful for any customers in generated code. This aspect will be made configurable in 6.0 branch for customers who want to see these diagnostics in generated code.

Right, thanks @mavasani for clarification

I did not mean the analyzer is not useful for generated code, but for me generated code is rarely gets updated/changed, so running the analyzer for each build would be redundant, better to be enabled when needed and off by default.

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