Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ linter #163

Merged
merged 17 commits into from
Feb 26, 2014
Merged

C++ linter #163

merged 17 commits into from
Feb 26, 2014

Conversation

jeffdonahue
Copy link
Contributor

Add Google's C++ linter (scripts/cpp_lint.py) with "lint" target in Makefile (run "make lint" to run the linter on all C source/header files) and fix all of its errors for the current codebase. The linter statically checks C++ code against some of the C++ style conventions Caffe follows, heavily inspired by Google's C++ style guide.

If this PR is accepted, all future Caffe contributions should be required to run "make lint" with an output of "Total errors found: 0" before merging (returns status code 0 in this case). In some cases this may involve explicit suppression of certain false positive linter errors (or when there's just a good enough reason to break convention) using NOLINT and NOLINT_NEXTLINE (the latter of which I added to the cpp_lint.py script due to the same line NOLINT almost always making the line run over 80 characters).

@sguada
Copy link
Contributor

sguada commented Feb 26, 2014

Wow these are a lot of changes, I hope it doesn't break any of the other PR, specially the one @shelhamer is doing in dev

@shelhamer
Copy link
Member

@sguada it's ok. We're going to merge the re-arrangement first then check this out. Having a clean cup of Caffe with these changes will be nice. Thanks @jeffdonahue.

@jeffdonahue
Copy link
Contributor Author

this is good to go (barring any further need to rebase)

@shelhamer
Copy link
Member

I vote we merge this now, then do another lint pass once #165 is in. I don't want this to be relegated to dev and merging it now lets us bring it into master along with #142 now.

@jeffdonahue ok with you?

@jeffdonahue
Copy link
Contributor Author

Yup, sounds good to me

@shelhamer shelhamer merged this pull request into BVLC:dev Feb 26, 2014
@jeffdonahue jeffdonahue deleted the linter branch February 26, 2014 06:07
@sguada
Copy link
Contributor

sguada commented Feb 26, 2014

@jeffdonahue great job

It works great, I already found some "errors" in my code.

shelhamer added a commit that referenced this pull request Feb 26, 2014
Lint C++ (suffer for fashion)
shelhamer added a commit that referenced this pull request Feb 26, 2014
Lint C++ (suffer for fashion)
@shelhamer shelhamer mentioned this pull request Mar 18, 2014
mitmul pushed a commit to mitmul/caffe that referenced this pull request Sep 30, 2014
Lint C++ (suffer for fashion)
lukeyeager pushed a commit to lukeyeager/caffe that referenced this pull request Jun 8, 2016
Moved file accidentally - moving it back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants