search

mc-imperial / gpuverify

GPUVerify: a Verifier for GPU Kernels
http://multicore.doc.ic.ac.uk/tools/GPUVerify/
Other
58 stars 15 forks source link

Add StyleCop.Analyzers support #11

Closed jeroenk closed 6 years ago

jeroenk commented 6 years ago

StyleCop.Analyzers is the moral successor of StyleCop.

I disabled a number of warnings:

Feel free to disagree with the above.

Even with the above rules disabled, we get about 2700 warnings (after having fixed around the same number already).

afd commented 6 years ago

On 02/12/2017 16:24, Jeroen Ketema wrote:

StyleCop.Analyzers is the moral successor of StyleCop.

I disabled a number of warnings:

  • Missing copyright header (our default copyright header is not detected)
  • Require a space after |//| (this does not work with our copyright header)

Agree with the above two, though we could always change our default header so that it is detected. It might be nice to ensure that we always have a header.

  • Enable the generation of XML documentation (there's hardly any in the code base)

Agreed.

  • Always require |this| when accessing a member of the current class (we generally do not do this and does not seem worth fixing)

Agreed.

  • Always require braces for compound statements (there are quite a few places where we violate this rule, and I do not think it always improves readability of the code)

I disagree with this one - personally I like to always have braces for compound statements. Your call.

Feel free to disagree with the above.

See above :-)

A

Even with the above rules disabled, we get about 2700 warnings (after having fixed around the same number already).

    You can view, comment on, or merge this pull request online at:

https://github.com/mc-imperial/gpuverify/pull/11

    Commit Summary

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mc-imperial/gpuverify/pull/11, or mute the thread https://github.com/notifications/unsubscribe-auth/AEhSXf6ngfR8BKtI45E2V2t9RfbatcmNks5s8XnHgaJpZM4QzXQt.

jeroenk commented 6 years ago

I disagree with this one - personally I like to always have braces for compound statements. Your call.

I debated this one with myself for quite a bit here. Usually I would always write them, but StyleCop also enforces the following formatting:

if (...)
{
  ...
}

So it can get a bit unreadable at some point.

Note that for something like:

foreach (...)
  if (...)
    ...

StyleCop will still warn, and suggest braces for the foreach, because the the if-statement is multiline (this warning I left on).

jeroenk commented 6 years ago

Note that in the commit I added yesterday, I turned off several more warnings: multiple related to missing parts of xml documentation, and the warning that says that with a Debug.Assert you should always provide not only a condition, but also a string with an error message. I actually like the latter warning a lot, but do not think we will realistically fix all these warnings.

jeroenk commented 6 years ago

I've disabled a few more rules (any feedback welcome):

jeroenk commented 6 years ago

With be above and my latest changes, we compile cleanly.

afd commented 6 years ago

On 05/12/2017 23:28, Jeroen Ketema wrote:

I've disabled a few more rules (any feedback welcome):

  • The rule that warns about multiple classes per file. This does not make a lot of sense for, e.g., the expression trees that we have in our interpreter.
  • The rules that enforce a specific ordering for the members of a class, e.g. static methods before non-static ones. Although these rules make some sense, we have situations where the rules would require members that logically belong together to be far apart.

I'd keep the latter rule - what you say likely means that we should be splitting some big classes up into smaller classes, which might be a good thing.

A

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mc-imperial/gpuverify/pull/11#issuecomment-349476377, or mute the thread https://github.com/notifications/unsubscribe-auth/AEhSXYX-JKwQujBHwlmc_LF9V-BBPYLrks5s9dG5gaJpZM4QzXQt.

afd commented 6 years ago

Nice!

A

On 05/12/2017 23:29, Jeroen Ketema wrote:

With be above and my latest changes, we compile cleanly.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mc-imperial/gpuverify/pull/11#issuecomment-349476525, or mute the thread https://github.com/notifications/unsubscribe-auth/AEhSXezAJ1d_WEJvdnqjCNN3EBg-jcTzks5s9dHjgaJpZM4QzXQt.

jeroenk commented 6 years ago

I'm going to disagree with re-enabling the ordering rules, because it leads to stupid stuff like:

And, yes, large classes probably do need to be refactored in a number of smaller ones.

Question: StyleCop now just produces warnings. I could raised that to the level of errors. Not sure what you do in other projects?

afd commented 6 years ago

Ah, I had only been thinking about fields. I agree with you regarding methods.

Yes, we raise to errors so that the project’s CI fails - this forces the style to be kept up.

A

From: Jeroen Ketema notifications@github.com Sent: 06 December 2017 07:57:14 To: mc-imperial/gpuverify Cc: Donaldson, Alastair F; Comment Subject: Re: [mc-imperial/gpuverify] Add StyleCop.Analyzers support (#11)

I'm going to disagree with re-enabling the ordering rules, because it leads to stupid stuff like:

And, yes, large classes probably do need to be refactored in a number of smaller ones.

Question: StyleCop now just produces warnings. I could raised that to the level of errors. Not sure what you do in other projects?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/mc-imperial/gpuverify/pull/11#issuecomment-349563033, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEhSXVW3gjL03vpLEhmU4N21GjboTkfPks5s9kjagaJpZM4QzXQt.

jeroenk commented 6 years ago

I've merged this under the assumption that we can always re-evaluate the disabled rules.