Various issues found by lgtm.com

[v1993]

I've decided to take a look at OpenBVE analysis by lgtm.com and it seems to include some useful information about potential issues. "Read more" often provides very useful information about warning type.

I'm not a C# expert, but here are some of my through about warnings:

• Container contents are never accessed - cases look suspicious. Might indicate dead/unfinished code or logic error.
• Off-by-one comparison against container length - should be trivial to fix.
• Possible loss of precision - most likely not really important for now, but worth taking a look later.
• Potentially dangerous use of non-short-circuit logic (now that's a LOT of warnings!) - triggered by using bitwise AND/OR (&/|) instead of logical ones (&&/||) in conditions. Should be trivial to fix, but there are really a lot of them.
• Recursive call to Equals(object) - single alert looking like some sort of oversight.
• Call to GC.Collect() - ignore for now.
• Dereferenced variable may be null - mostly missing null checks I suppose?
• Dubious downcast of 'this'/Dubious type test of 'this' - it's more about slightly weird type usage by OpenBVE code if I got it right.
• Empty branch of conditional, or empty loop body - maybe those were intended? They still look weird.
• Missing Dispose call on local IDisposable - look like resource leaks. Make sure to "Read more" on warning.
• Use of default ToString() - notes useless debug messages.
• Useless assignment to local variable - some are clearly intended, others might not be.
• Class implements ICloneable - description might be worth a read, but it is not important as an issue.
• Poor error handling: empty catch block - doesn't really look useful. Maybe check a lot later.
• String concatenation in loop - performance-related. Using StringBuilder might improve it by noticeable amount.
• Too many 'ref' parameters - doesn't look quite applicable, at least not at first.

[leezer3]

Going to have to look at that in more detail, however for starters: Container contents are never accessed Known and deliberate. At the minute, we only support some (but not all) of the features of certain filenames. In all these cases, it's parsing / validation of unused features.

[v1993]

@leezer3 What about this case? https://lgtm.com/projects/g/leezer3/OpenBVE/snapshot/94f085d3629ecd004692f6fce532c3d8c8a18ba0/files/source/InputDevicePlugins/SanYingInput/ConfigForm.cs?sort=name&dir=ASC&mode=heatmap#xf09667f4fbebccc8:1

[s520]

@v1993 The code is still the same as it was written in the code of the program from which it was ported. I see no problem in removing it.

[shooking]

Just a thought - have the reported issues been debugged and shown to be true? I deal with static analysis on a daily basis. It's great ... except false positive and negatives can be triggered.

C# analysers are quite poor - can get more from inbuilt MS code analyser.

Not trying to suggest for one moment not to make changes - but just some caution towards coding to a SAST tool - they work better if you add rules to educate about your coding style.

My 2 cents.

On Thu, 27 Aug 2020, 19:44 s520, [email protected] wrote:

The code is still the same as it was written in the code of the program from which it was ported. I see no problem in removing it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/leezer3/OpenBVE/issues/521#issuecomment-682124655, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFO7EV2B5JZK4P5ZJ3HIGO3SC2SRJANCNFSM4QNGOQTQ .

[leezer3]

Absolutely.

The biggest single trouble with code-style in this project is that what we first inherited was frankly a mess. Whilst things work, we're still trying to get things in order (Case in point- It took over four years to get as far as moving the route parser into a shared plugin and rebuilding it into less of a massive lump...)

This is an initial quick run through and pulling out of 'real' issues: https://github.com/leezer3/OpenBVE/tree/LGTM

With the non-short-circuit logic, I strongly suspect this was the reason for this originally:

If your code is performance sensitive enough and the operations cheap enough, using the non-short circuit can be faster. This is because using || involves performing a branch, and a branch prediction miss can be very expensive. Where as the | performs a calculation and examining a variable can be much faster, avoiding a branch prediction miss.

Remember that a lot of this code is 10+ years old, and execution was slower, and the Mono compiler considerably worse. Can probably be changed, but be wary of side effects.