Improve Rule S2223: cleanup and performance

[mary-georgiou-sonarsource]

RSPEC PR: SonarSource/rspec#1583

Performance comparison:
pascalabcnet and servuo were the two projects with most issues for rule S2223 on peach.

Project	Before	After	Performance Improvement
pascalabcnet	544 issues - 1,694 s	544 issues - 1,262ms	-25.5%
servuo	437 issues - 4,305 s	437 issues - 2,988ms	-30.5%

[andrei-epure-sonarsource]

I suggest to test the performance impact (before and after) - see this page and/or the msbuild -p:reportanalyzer=true parameter

To get the AnalyzerRunner, you need to build this project locally.

[antonioaversa]

Minor comments.

The only main aspect is the private protected visibility, which would possibly (but not necessarily) be a false negative, since this visibility is almost private as private itself.

[martin-strecker-sonarsource]

A volatile field should also be irrelevant as volatile indicates some advanced multi-threading voodoo is going on here.

[mary-georgiou-sonarsource]

maybe good to exclude - per MSDN docs

The volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time.

On a multiprocessor system, a volatile read operation does not guarantee to obtain the latest value written to that memory location by any processor.

[antonioaversa]

Ok, while I see a public static volatile non-readonly field as dangerous as a public static non-volatile non-readonly one, and it seems that there are almost always ways to have the field private (at least for the typical scenarios of sentinel loopers or counters), let's assume that the person using it has a sound multi-threading model and a solid understanding of it.

So I am OK with disabling the check when volatile is encountered.

[antonioaversa]

@martin-strecker-sonarsource to resolve this one

[mary-georgiou-sonarsource]

@martin-strecker-sonarsource and I had an offiline convertation on the matter and agreed to the current solution regarding volatile modifier.

[mary-georgiou-sonarsource]

@antonioaversa I added also the volatile keyword as an exception as from the msdn docs my understanding is that when a field is declared volatile it is expected that the values might change in an unexpected way and might be an FP?

On a multiprocessor system, a volatile read operation does not guarantee to obtain the latest value written to that memory location by any processor.

[antonioaversa]

LGTM! Just minor things, mostly aesthetics.

[antonioaversa]

It would be good to align vertically all these // Noncompliant.

[antonioaversa]

Ok, while I see a public static volatile non-readonly field as dangerous as a public static non-volatile non-readonly one, and it seems that there are almost always ways to have the field private (at least for the typical scenarios of sentinel loopers or counters), let's assume that the person using it has a sound multi-threading model and a solid understanding of it.

So I am OK with disabling the check when volatile is encountered.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication