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

Fix cpplint's NOLINT() with new rule names #635

Merged
merged 1 commit into from
Oct 7, 2015

Conversation

cbagwell
Copy link
Contributor

NOLINT() only supports original rules list from _ERROR_CATEGORIES
but that list was left unmodified by cpplint_creatrules.py script.
This meant that any existing code that developers marked as false
positives using NOLINT method began reporting errors when switching to
modified cpplint and also it was impossible to re-mark using new rule names.

This modifies NOLINT() behavior to work like cpplint's --filter option
which masks any rule that starts with specified pattern.

This allows, for example, "// NOLINT(build/c++11)" to mask build/c++11
rule from non-modified cpplint as well as build/c++11-0 in modified
cpplint.

@jmecosta
Copy link
Member

Thanks for the PR, I will try this as soon as possible. There are so check broken it might just be travis . you can try to push something to see if it passes

@cbagwell
Copy link
Contributor Author

Thanks for the quick feedback. I looked for a while in travis logs to feel comfortable that the boost test failure is not likely to be related to PR. I'll go read up best way to restart travis job.

In mean time, here is a small test C++ file to run with cpplint.py and cpplint_mod.py to demonstrate the issue before this PR. The rule for using unapproved library becomes build/c++11-2 in modified version of cpplint. 2nd line is correctly disabled when ran with cpplint.py but all three show up as issues with cpplint_mod.py.

#include <thread>
#include <thread>  // NOLINT(build/c++11)
#include <thread>  // NOLINT(build/c++11-2)

NOLINT() only supports original rules list from _ERROR_CATEGORIES
but that list was left unmodified by cpplint_creatrules.py script.
This meant that any existing code that developers marked as false
positives using NOLINT method began reporting errors when switching to
modified cpplint and also it was impossible to re-mark using new rule names.

This modifies NOLINT() behavior to work like cpplint's --filter option
which masks any rule that starts with specified pattern.

This allows, for example, "// NOLINT(build/c++11)" to mask build/c++11
rule from non-modified cpplint as well as build/c++11-0 in modified
cpplint.
@jmecosta
Copy link
Member

OK now I got it... Haven't come across this since usually we don't like
polluting code with additional NO lint in code. Thanks for the example

On Thu, Sep 24, 2015, 05:05 Chris Bagwell [email protected] wrote:

Thanks for the quick feedback. I looked for a while in travis logs to feel
comfortable that the boost test failure is not likely to be related to PR.
I'll go read up best way to restart travis job.

In mean time, here is a small test C++ file to run with cpplint.py and
cpplint_mod.py to demonstrate the issue before this PR. The rule for using
unapproved library becomes build/c++11-2 in modified version of cpplint.
2nd line is correctly disabled when ran with cpplint.py but all three show
up as issues with cpplint_mod.py.

#include
#include // NOLINT(build/c++11)
#include // NOLINT(build/c++11-2)


Reply to this email directly or view it on GitHub
#635 (comment).

@jmecosta
Copy link
Member

@cbagwell i cannot get this thing to work, // NOLINT alone works, but with the patched version i cannot get // NOLINT(readabilitiy/casting-1) to be ignored

what version are u using of cpplint?

@jmecosta
Copy link
Member

@cbagwell ok, now it works, just realize my version is really old...need to update then.

this can be merged. and thanks for the fix

@cbagwell
Copy link
Contributor Author

To be safe, I looked at cpplint.py commit log and looks like code this PR applies to was added in Apr 30, 2010. I tested PR on that version and test worked. Seems reasonably long enough time period to not worry about asking people to upgrade.

@guwirth guwirth added this to the M 0.9.4 milestone Oct 6, 2015
@guwirth
Copy link
Collaborator

guwirth commented Oct 6, 2015

@cbagwell final version ready to merge?

@cbagwell
Copy link
Contributor Author

cbagwell commented Oct 7, 2015

@guwirth Yes, its final version and ready to merge.

guwirth added a commit that referenced this pull request Oct 7, 2015
Fix cpplint's NOLINT() with new rule names
@guwirth guwirth merged commit 46d25ed into SonarOpenCommunity:master Oct 7, 2015
@guwirth guwirth mentioned this pull request Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants