ls1mardyn / ls1-mardyn

ls1-MarDyn is a massively parallel Molecular Dynamics (MD) code for large systems. Its main target is the simulation of thermodynamics and nanofluidics. ls1-MarDyn is designed with a focus on performance and easy extensibility.
http://www.ls1-mardyn.de
Other
28 stars 15 forks source link

Fix bug in static code analysis and add exit codes #322

Closed HomesGH closed 4 months ago

HomesGH commented 4 months ago

Description

Fix a bug in the static code analysis tool and set proper exit codes to make the execution of the script meaningful. The bug excluded all header files from being scanned. This PR also fixes the detected files by adding new lines at the end of the file and changing the line endings from CRLF.

Resolved Issues

HomesGH commented 4 months ago

While we are at it we could also add a check whether files use tabs or spaces ;) Something like grep -e "^[[:space:]]\+" $file

Yes, that's a good idea in principle! But since every file is checked and not just the changed ones, this will lead to a lot of output :sweat_smile: Regarding the specific command: Wouldn't this not discriminate between blank space and tab? Shouldn't it be something like this:

# Count lines leading with tabs
numtabs=$(grep -P '^\t' "$file" | wc -l)

# Count lines leading with two or more spaces
numspaces=$(grep -P '^ {2,}' "$file" | wc -l)

if [ "$numtabs" -gt 0 ] && [ "$numspaces" -eq 0 ]; then
  echo "$file uses tabs for indentation."
elif [ "$numtabs" -eq 0 ] && [ "$numspaces" -gt 0 ]; then
  echo "$file uses spaces for indentation."
elif [ "$numtabs" -gt 0 ] && [ "$numspaces" -gt 0 ]; then
  echo "$file uses both tabs and spaces for indentation."
else
  echo "$file does not use any consistent indentation."
fi
FG-TUM commented 4 months ago

Ah my bad, I forgot that [[:space:]] includes tabs :man_facepalming:. So what I meant was rather grep -e "^ \+" $file.

I don't think a check needs to be as complicated as your snippet. If we only restrict ourselves to .cpp and .h files, anything that starts with a space is invalid, so there is no need to count space / tabs lines.

But overall, I agree that this would blow up this PR. Also, instead of building 1000 checks like this ourselves, it would make more sense to just use clang-format or similar tools. Hence I'll merge this PR as it is.