Add clint for checking coding style

[davidzchen]

Adding a lint script for checking source files for coding style.

The new neovim coding style, as decided in the poll in #104, is a modification of the Google C++ coding style with the following modifications:

• Function names are lower_case rather than PascalCase
• Struct and enum names that have not been typedef-ed are struct lower_case and enum lower_case.
• Opening braces for functions appear on the next line.

This PR is for adding Google's cpplint.py lint script and modifying it with the above changes. We will also do some trial conversions of some source files to determine whether additional tweaks are required. Due to the additional time required to make modifications to cpplint.py, these additional tweaks should be limited and in favor of adhering more closely to the Google convention than diverging from it.

[davidzchen]

Done:

• Add .c to the list of valid file extensions.
• Suppress error to use C++-style casts for .c and .h files.

Some observations:

• Lint apparently does not check for the format of function names.

To do

Moving opening braces onto the next line

This might be non-trivial due to the way that cpplint checks for braces. It checks whether there is an opening brace on its own line and errors out. It only allows this case if:

...the last non-whitespace character on the previous non-blank line is ',', ';', ':', '(', '{', or '}', or if the previous line starts a preprocessor block.

I do not want to make significant arbitrary changes, but I think given that we want to finish this as soon as possible and that the very close second and third choices put the brace at the end of the function declaration, if modifying cpplint for this turns out to be too difficult

Trial conversions

I'm going to do trial conversions of a few files to determine whether additional modifications are necessary. I see that some files have gotos but have yet to see whether this will fail lint since the style guide does not mention goto.

[simendsjo]

Just remove the check for { on a blank line and add a new ticket to fix it sometime later. It doesn't have to be perfect for the first run :)

[davidzchen]

Good idea. :) Changed. All of the 3 modifications are now covered. I'll look for any additional cases that need to be handled.

[rjw57]

AIUI this PR is a work in progress and needs to be finished before being merged, is that correct or have I misunderstood the "To do" secion?

[davidzchen]

The PR is a work in progress; I opened it early for discussion. The first to do item is done. The remaining item is to see whether there are any more obvious cases that require modifying the lint script in order to handle.

[rjw57]

@davidzchen Thanks for clarifying. When it's ready, could you @-mention one of people with merge access so we know when to look over it? I like the idea of something which can check the code style which has been agreed on. (Think: the checkpatch.pl script in Linux.) This looks to be moving in roughly the right direction although I'd prefer it if it didn't add quite so many lines :). Is there a way to import a pip installable library/tool and just include a config file?

[davidzchen]

@rjw57 Thanks. Will do.

Some common errors

I have run clint.py on some of the source files. There are a few common errors that we may or may not want to suppress:

./undo.c:99: Include the directory when naming .h files [build/include] [4]

The Google C++ coding style specifies that source code under src should be organized into modules as subfolders, and that all header files should be included relative to src. This particular check fails when an #include "foo.h" directive does not contain any /'s. Are we planning to do any such restructuring of the source tree? If not, then I think this check should be suppressed.

./buffer.c:4345: Use int16/int64/etc, rather than the C type long [runtime/int] [4]

This error is raised due to usage of long. I think that it is a good idea to clean these up and replace them with the stdint.h types since long differs among different platforms.

./src/blowfish.h:0: No copyright message found. You should have a line: "Copyright [year] " [legal/copyright] [5]

Does neovim have a copyright owner? Are we planning to change the header comment in the source files? If we are not, I'll suppress this error.

Other observations

• Lint does not check for gotos, nor does it specify the style that labels should follow. I propose that labels should also follow the lower_case style, which most of the labels currently follow.
• Currently, clint.py will throw a large number of errors for most of the source files, which is not unexpected. A good number of them are the two specified above and formatting issues that we should be able to fix using uncrustify. I think that initially, we should run clint.py manually to help with code review. Cnce the codebase is at a more manageable state, we can consider automatically running it such as invoking clint.py at build time.

[davidzchen]

@tarruda, @simendsjo, @rjw57 Thoughts on the above?

[simendsjo]

Just a general note: I think it's fine to remove rules from the linter, especially in a (long) transition period. We should pick up this thread again after uncrustify has been run. @davidzchen You are writing the uncrustify config too?

[tarruda]

@davidzchen that's great. At first I thought about using uncrustify for checking style(apply and if it differs generate an error) but this is much better as it will report the exact errors.

[jszakmeister]

@davidzchen Just an FYI, when trying to use uncrustify a couple years ago I ran into a number of edge cases it didn't handle well. Just be careful using it, and make sure to go over the results.

[davidzchen]

Thanks. I'll suppress the three errors I listed above. When we decide to clean those up, we can enable them individually for checking those tasks.

I have been experimenting with uncrustify. It's a neat tool, but as @jszakmeister mentioned, there are certain edge cases that it does not handle very well. One of the issues I have been running into is caused by the fact that many comments are currently at the end of a line. There might be a configuration option that would help, but I am still going through all the options.

Once I have an uncrustify config that works, I'll add it, as well as some documentation changes to CONTRIBUTING.md.

[davidzchen]

I have added my in-progress uncrustify.cfg. I have most of the wrinkles smoothed out. There are a few remaining issues such as variable declarations with *'s still getting aligned and switch statements not being spaced correctly.

Once I'm done, I'll squash all of these changes using rebase.

[gilligan]

Users of syntastic might find this useful: https://gist.github.com/gilligan/9326904

It adds clint as a possible checker for c files. Note that clint.py needs to be in your path and that you will have to modify g:syntastic_c_checkers because it'll otherwise use gcc or make as default. See the syntastic documentation for details on that.

[davidzchen]

I think the uncrustify.cfg is as good as it gets for now without spending a further inordinate amount of time on it.

I have squashed all of my commits and rebased on master. I think this PR is ready to be merged. Once this is merged, I will use uncrustify.cfg to make an initial style cleanup pass and submit that as a separate PR.

[simendsjo]

This pull is for both clint.py and uncrustify.cfg, right?
I'm trying to test clint.py without much success. It doesn't complain about anything, even if I add a new global variable 16 tabstop out with a bad name.

[davidzchen]

Yes, this is for clint.py, uncrustify.cfg, and changes to CONTRIBUTING.md with a note on the coding style.

That is interesting. What is your input file? I downloaded the clint.py from the diff view and it still works for me.

[simendsjo]

Maybe I'm not using clint.py correctly?

wrong.c:

        #define someDefine 1
            int someVariable    =   1   
    ;

./clint.py wrong.c <- no output

[davidzchen]

That is interesting. This is the output I get:

dzc@apogee ~> ./clint.py foo.c 
./foo.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
./foo.c:2:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
./foo.c:3:  Line contains only semicolon. If this should be an empty statement, use {} instead.  [whitespace/semicolon] [5]
Done processing ./foo.c
Total errors found: 3

Do you get output when you run ./clint.py --help? If not, which Python version are you running?

[mahkoh]

#!/usr/bin/python

That's bold. How about

#!/usr/bin/python2

[simendsjo]

@davidzchen : clint.py --help works.

➜  ~  python --version
Python 3.3.4
➜  ~  python2 --version
Python 2.7.6

[davidzchen]

@mahkoh Apparently Google uses Python 2.x internally.

@simendsjo If you change the shebang to #!/usr/bin/python2 does clint.py now work?

[simendsjo]

On 03/04/2014 06:57 PM, David Z. Chen wrote:

@mahkoh Apparently Google uses Python 2.x internally.

@simendsjo If you change the shebang to #!/usr/bin/python2 does
clint.py now work?

—
Reply to this email directly or view it on GitHub.

Yes, it works with python2

[davidzchen]

Is there a way to ensure that this script will be run using python2 for users who have selected Python 3 as their primary Python version without changing the shebang to #!/usr/bin/python2? I only have Python2 on my system and as a result #!/usr/bin/python2 does not exist.

[davidzchen]

I think this PR is ready to be merged.

I have not implemented checking for braces for control structures in clint, but the combination of that and testing is non-trivial and uncrustify will automatically insert braces anyway. Likewise, clint also does not check for the opening brace for function declarations being on the next line, but as @simendsjo mentioned earlier in this thread, these changes can come later.

After this PR is merged, I would like to make an initial cleanup pass with uncrustify and will submit that as a separate PR.

[dpelle]

a open brace → an open brace

[dpelle]

preceeded → preceded

[davidzchen]

@dpelle I appreciate your diligence, but these comments are actually automatically generated by uncrustify --update-config-with-doc. As a result, I don't think it would be worth the effort to clean up all the grammatical errors in these comments.

[davidzchen]

@simendsjo @tarruda Do you have any more feedback on this before it can be merged?

[mahkoh]

One thing I noticed is that it complains about variables named string.

[tarruda]

@davidzchen this looks good to me. Can you rebase on top of master? Also make sure any necessary dependencies for travis are installed(before_install in travis.yml)

[davidzchen]

Thanks @tarruda. I have rebased on top of master. I want to make an initial pass with uncrustify in a separate PR after this. Because clint will throw a large number of errors right now, I have not integrated clint with the build system and as a result, there are no changes to travis needed at this point.

[tarruda]

Merged. @davidzchen I think before merging clint you will need to pipe all files through uncrustify right?

[mahkoh]

@davidzchen:

Due to the additional time required to make modifications to cpplint.py, these additional tweaks should be limited

There are some false positives. If you don't mind, I'm going to remove the parts that don't apply to C code (templates, references, etc.)

[davidzchen]

@tarruda Correct. Before we integrate clint into any sort of automation, we should at least do an initial clean-up pass. I have started doing an initial clean-up using uncrustify and will send a separate PR for that soon.

@mahkoh That sounds good. I'd be fine with the UNIX "do one thing and do it well" principle and have clint.py be specific to C. If we decide to start adding C++ to the codebase in the future, we can re-import cpplint.py and use that script specifically for the C++ code.