no windows newlines, please

[Bob Carpenter]

We're finding Windows newlines in some of the commits and want
to stick to unix-only.

I'll clean it all out before the next release, but please please
please,

* NO TABS IN CODE

* NO WINDOWS NEWLINE

And if you really want to make me nappy,

* NO SPACE BETWEEN FUNCTION NAME AND LEFT PAREN

* SPACE BETWEEN if and LEFT BRACKET

- Bob

[Allen B. Riddell]

I'd add trailing whitespace too.

For simple things like these, you can setup a pre-commit hook to prevent
these things from ever being committed, e.g.,
https://github.com/pre-commit/pre-commit-hooks

-a

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Daniel Lee]

Although we can suggest something like that, they're optional, correct?

If we can script it, we can have Jenkins or Travis-CI fail the pull request.

[Daniel Lee]

Here are the list of Jenkins plugins, look under "Build reports":
https://wiki.jenkins-ci.org/display/JENKINS/Plugins

If we can get checkstyle to work (http://checkstyle.sourceforge.net/), there's already a Jenkins plugin that will display output for us.

Alternatively, if you can get any of these tools to work on a Mac:
http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#C.2FC.2B.2B

We can set it up on the Jenkins machine and have it notify us when a patch is bad. The only requirement that Jenkins has is whatever it executes returns 0 when it's successful, non-0 when it fails.

Daniel

[Daniel Lee]

Any thoughts on Google's style? We can run cpplint.py on Jenkins to check for style. We can also use a subset of the rules implemented by cpplint.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

[Daniel Lee]

So, after installing cpplint, here's the summary output I get from the summary source tree:
> time cpplint.py --counting=detailed --root=src --extension=hpp $(find src/stan -name '*.hpp') 2>&1 | grep -e "Category" -e "Total error"
Category 'runtime/indentation_namespace' errors found: 1503
Category 'readability/casting' errors found: 63
Category 'whitespace/semicolon' errors found: 60
Category 'runtime/references' errors found: 856
Category 'whitespace/operators' errors found: 50
Category 'whitespace/comments' errors found: 514
Category 'build/include_what_you_use' errors found: 130
Category 'whitespace/line_length' errors found: 1130
Category 'legal/copyright' errors found: 800
Category 'build/namespaces' errors found: 4
Category 'build/include' errors found: 36
Category 'whitespace/empty_loop_body' errors found: 1
Category 'whitespace/end_of_line' errors found: 7290
Category 'runtime/int' errors found: 21
Category 'whitespace/comma' errors found: 4404
Category 'readability/namespace' errors found: 1588
Category 'whitespace/indent' errors found: 443
Category 'runtime/explicit' errors found: 205
Category 'whitespace/newline' errors found: 1532
Category 'readability/braces' errors found: 225
Category 'readability/todo' errors found: 6
Category 'build/include_order' errors found: 1100
Category 'whitespace/forcolon' errors found: 3
Category 'build/storage_class' errors found: 3
Category 'whitespace/blank_line' errors found: 841
Category 'whitespace/parens' errors found: 386
Category 'whitespace/braces' errors found: 116
Total errors found: 23310

real    0m37.399s
user    0m35.911s
sys    0m1.177s

It takes under a minute to run, so I think it's well worth it. I think we should filter out these error messages:
- runtime/indentation_namespace: wants there to be no indentation with namespaces. Since some of us use emacs to indent, this isn't the default.
- readability/namespace: wants "// namespace <name>" to end each namespace. We haven't been following this.
- legal/copyright: wants a copyright notice on each file

- whitespace/line_length: is set to 80 characters. Should we make it longer?

- whitespace/indent: wants 'public:' in class definitions to be indented by 1 space. There is a google style emacs file that could help with this sort of indentation. I'm ok without it indented.

Some key problems this will find:

- whitespace/newline: WINDOWS NEWLINES!

- whitespace/parens: "if(", "foo (", "for("

This I can go either way, but can suppress for now:

- runtime/reference: 

src/stan/agrad/autodiff.hpp:198:  Is this a non-const reference? If so, make const or use a pointer: Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H  [runtime/references] [2]

the function signature looks like:
    template <typename F>
    void
    hessian(const F& f,
            const Eigen::Matrix<double,Eigen::Dynamic,1>& x,
            double& fx,
            Eigen::Matrix<double,Eigen::Dynamic,1>& grad,
            Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H) {

and it wants grad and H to be pointers.  (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments)

For the time being, I'm going to set up a project that runs through the source code and we can see if the Jenkins output is useful.




Daniel

[Daniel Lee]

Ok, got something working. http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20-%20CppLint/

That'll show you a chart of warnings between builds. We've got ~18k right now. To get detailed information, click on the "CppLint Warnings" link on the left: http://d1m1s1b1.stat.columbia.edu:8080/job/Stan%20-%20CppLint/warnings9

You can click through and get the individual warning messages. I didn't include the test files.

Questions:

1. is this useful?

2. should we run this on every pull request?

3. should we use different settings?

4. should we run this on the test code? without autogenerated tests, this pushes the number to ~80k and makes Jenkins unbearably slow. if we were to go through this, my suggestion would be to get the numbers lower first, then start running it on the test when it's under 20k total.

Daniel

[Michael Betancourt]

It’s a little opaque how to translate this to the dev who submitted

the pull request.  Is there a way to email the detailed report, in

particular the “Details” page but for only those files touched by

the pull request?

In any case I’d say that we might as well roll this out for future

pull requests and not yet for tests.

[Michael Betancourt]

- readability/namespace: wants "// namespace <name>" to end each namespace. We haven't been following this.

I like requiring this (we have some long files), but I’m not wedded to it.

- whitespace/line_length: is set to 80 characters. Should we make it longer?

I’d say no more than 100.

- whitespace/indent: wants 'public:' in class definitions to be indented by 1 space. There is a google style emacs file that could help with this sort of indentation. I'm ok without it indented.

Mixing 1 and 2 space indents makes me sad.  :-(

src/stan/agrad/autodiff.hpp:198:  Is this a non-const reference? If so, make const or use a pointer: Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H  [runtime/references] [2]

the function signature looks like:
    template <typename F>
    void
    hessian(const F& f,
            const Eigen::Matrix<double,Eigen::Dynamic,1>& x,
            double& fx,
            Eigen::Matrix<double,Eigen::Dynamic,1>& grad,
            Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H) {

and it wants grad and H to be pointers.  (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments)

But that changes the whole interface!  What a wacky error.

[Allen B. Riddell]

I would support enforcing some style guidelines. Maybe there's a subset
of the Google style that everyone can agree on (e.g., no trailing
whitespace, no windows newlines) and that can be enforced via a linter.

Thanks,

-a

[Daniel Lee]

I'm with Mike on everything. 

I'd suggest we run it for a while without enforcing it and manually looking at the output of it to suggest changes to pull requests. Once we get used to it and we fix some of it, we can think about which rules we want to enforce and we'll doc it.

Daniel

[Bob Carpenter]

> On Jan 27, 2015, at 4:56 AM, Michael Betancourt <betan...@gmail.com> wrote:
>
>
>> - readability/namespace: wants "// namespace <name>" to end each namespace. We haven't been following this.
>
> I like requiring this (we have some long files), but I’m not wedded to it.
>

I'm OK either way.

>> - whitespace/line_length: is set to 80 characters. Should we make it longer?
>
> I’d say no more than 100.

I like 80 as the warning limit, but could live with 100.

>> - whitespace/indent: wants 'public:' in class definitions to be indented by 1 space. There is a google style emacs file that could help with this sort of indentation. I'm ok without it indented.
>
> Mixing 1 and 2 space indents makes me sad. :-(

I agree. WTF ar they thinking?

>> src/stan/agrad/autodiff.hpp:198: Is this a non-const reference? If so, make const or use a pointer: Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H [runtime/references] [2]
>>
>> the function signature looks like:
>> template <typename F>
>> void
>> hessian(const F& f,
>> const Eigen::Matrix<double,Eigen::Dynamic,1>& x,
>> double& fx,
>> Eigen::Matrix<double,Eigen::Dynamic,1>& grad,
>> Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>& H) {
>>
>> and it wants grad and H to be pointers. (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments)
>
> But that changes the whole interface! What a wacky error.

Agree. WTF #2.

- Bob