Lint in Astrobee

[oleg-alexandrov]

The astrobee code passes the code through a linter when committing, which usually results in lots of complaints about formatting. Some of the formatting issues are fixed automatically by clang-format-8, but it can't fix such obvious things as whitespace at the end of lines, empty lines, missing space before brace, etc.

I propose adding a homegrown python tool to fix these. With such a refactoring tool one risks the danger that one's precious code gets messed up and lost, so the tool I have in mind would be very conservative. It would take as input the linter's output in a text file, which looks like this:

code.cc:1026: Line ends in whitespace. Consider deleting these extra spaces. code.cc:1034: Missing space before { code.cc:968: At least two spaces is best between code and comments code.cc:939: Redundant blank line at the start of a code block should be deleted.

It will first make a backup file called code.cc.bk, then modify precisely the lines the linter complains about while not touching anything else. (When it comes to blank lines, those will be flagged for wiping but will be removed after everything else, to not mess up the bookkeeping.)

I propose to also add to the "Contributing" section at:

https://github.com/nasa/astrobee#contributing

a link to a document describing our style, how to get clang-format-8, and how to use the little python tool as a second step.

Any thoughts?

This will go to the related ISAAC repo as well.

[rsoussan]

Hi Oleg, which config file are you using for clang-format-8? I copied the clang format used by the linter and haven't had issues except for having to manually add missing headers. Let me know if you'd like me to send the file to you to test out.

[rsoussan]

Nvm, I realize you're using the output of the githook. I wonder if that somehow behaves differently, I'll send you the config and let me know if that fixes your issues.

[oleg-alexandrov]

Your clang-format-8 config file seems to be identical to what astrobee uses already. I tried it with the command:

clang-format-8 -style=file -i myfile.cc

and it does not remove these:

Line ends in whitespace. Missing space before { Redundant blank line at the start of a code block

It does remove a lot of other stuff though.

Could you make a file with a space at the end and see if it wipes it for you?

[oleg-alexandrov]

It looks that clang-format-8 won't remove trailing spaces. https://stackoverflow.com/questions/44482062/removing-trailing-spaces-with-clang-format?rq=1

[rsoussan]

Hi Oleg, I added those items to a cc file and ran clang and the file reverted back to its normal state, I'm not sure what we're doing differently. Can you send me the cc file you're testing with so I can confirm if it fixes the file you're having issues with?

[oleg-alexandrov]

I figured out what is going on. The clang-format-8 program refuses to clean code within #if 0 blocks, but cpplint complains about those. I can confirm that clang-format-8 cleans up the code once such statements are removed.

[marinagmoreira]

One think to keep in mind too is that the clang-format-8 that runs with the githook, only formats the error lines. It was specified like this to avoid cases where we align the code to be more readable and the program destroys it, even if it is not a problematic area.

[oleg-alexandrov]

This is a little dangerous. If multiple passes of clang are used and some of them wipe lines, the bookkeeping for the range of lines will be wrong.

[marinagmoreira]

Well, because the linter only runs on the problematic lines, empty lines will not be fixed (by their own there is no lint error)

[rsoussan]

We could run clang on the whole code base once people have merged their large outstanding prs so that the rest of the code is properly formatted, then future clang githook checks can be on all code

[oleg-alexandrov]

If we run the code on the whole base to start with, it does not make sense to run it on pieces. People may think their code is safe when clang is not run on some chunks now, and later it will be messed up anyway.

The clang formater can be quite aggressive on things like messing up whitespace alignment and rewriting function prototypes by packing as many arguments as what fit on a given line which is terrible for readability.

[oleg-alexandrov]

My concerns were satisfied by the recent pull requests.

The documentation now says that the code to be contributed must be clean according to cpplint, that clang-fortran-8 is used to do the cleanup with a provided config file, and it was verified to work.

Wording was also added to the hook which does the cleanup saying that the commit should be attempted again in case clang-format-8 fixed the errors cpplint complained about.