stride icon indicating copy to clipboard operation
stride copied to clipboard

Address hundreds of compiler warnings

Open jrinker03 opened this issue 3 years ago • 2 comments
trafficstars

Release Type: GitHub

Version: master

Platform(s): build

Describe the bug When building the main stride solutions, there are hundreds of warnings emitted by the various tools. As of this issue submission, there are ~290 such warnings. This is fairly dangerous as it leads to developers ignoring potential bugs.

As examples of potential issues:

  • warning CS8073: The result of the expression is always 'false' since a value of type 'Guid' is never equal to 'null' of type 'Guid?'
  • warning C4244: 'initializing': conversion from 'double' to 'float', possible loss of data
  • warning C4244: 'argument': conversion from 'float' to 'const fbxsdk::FbxLongLong', possible loss of data

These may be benign but perhaps not. The safest solution is to examine each and address the warning in an appropriate way.

Additionally, there are reasonably harmless messages but point at clutter that needs to be addressed to streamline the codebase. Things like:

  • warning CS0162: Unreachable code detected
  • The variable 'e' is declared but never used
  • The field 'foo' is never used
  • 'some class or field' is obsolete

And the last class is use of deprecated items that the compiler points out. Deprecated classes are usually replaced by something that is superior in some way (for example, security or perf are high value reasons). Granted, these are likely benign at the moment, but could come back to cause issues later.

See also #1381

To Reproduce Steps to reproduce the behavior:

  1. Compile the stride.sln from the latest master code.

Expected behavior No warnings on build. Bonus would be that warnings are treated as errors.

Screenshots N/A

Log and callstacks See above for examples

Additional context I realize that these might be time consuming to address completely and I am willing to take these on over a series of PRs. From experience, having someone scrutinize these and bring up interesting cases for discussion is well worth the effort.

jrinker03 avatar Mar 27 '22 01:03 jrinker03

I see there's even more warnings on the build server. There's SA* warnings - which leads to believe the target that turns off analyzers by default is actually disabled on the build server.

manio143 avatar Mar 28 '22 21:03 manio143

From experience, some of those analyzer warnings are really good, too. Shame they're off locally. I'll have to take a peek at them one day.

jrinker03 avatar Mar 29 '22 01:03 jrinker03