Skip to content

--suppress on line 0#1354

Merged
PKEuS merged 9 commits into
cppcheck-opensource:masterfrom
shikamu:master
Sep 18, 2018
Merged

--suppress on line 0#1354
PKEuS merged 9 commits into
cppcheck-opensource:masterfrom
shikamu:master

Conversation

@shikamu
Copy link
Copy Markdown
Contributor

@shikamu shikamu commented Aug 29, 2018

added the ability to use suppress on line 0, since cppcheck can yield syntax error on line 0.

added the ability to use suppress on line 0, since cppcheck can yield syntax error on line 0.
@shikamu
Copy link
Copy Markdown
Contributor Author

shikamu commented Aug 29, 2018

Hello, I discussed quite extensively with PKEuS about this issue. it is possible for cppcheck to suppress syntax errors, you can run it with --suppress=syntaxError, however cppcheck can also give you an error that several people have encountered which is a syntaxError on line 0, and this cannot be suppressed because line 0 was not recognized by cppcheck as a valid line.

Comment thread lib/suppressions.cpp Outdated

if (suppression.lineNumber > 0) {
if (suppression.lineNumber >= Suppressions::Suppression::NO_LINE) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the >= should be >

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are correct, good catch!

Copy link
Copy Markdown
Collaborator

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be simplified

Comment thread test/testsuppressions.cpp Outdated
e.errorMessage,
e.id,
false);
cppCheck.reportErr(errmsg);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if you really need to test this with the entire CppCheck and Tokenizer.

Can't you just write something like:

void suppressionsLine0() {
    Suppressions suppressions;
    suppressions.addSuppressionLine("syntaxError:*:0");
    ASSERT_EQUALS(true, suppressions.isSuppressed(errorMessage("syntaxError", "test.cpp", 0)));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah perhaps, this is my first contribution to cppcheck so I'm not very familiar with it yet :D perhaps your code works! I'll check it in a week after my holidays!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is my first contribution to cppcheck so I'm not very familiar with it yet

Yes of course! I totally understand this. Sorry, I meant no disrespect at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't ;) now that I see your proposal more clearly, it makes a lot of sense 👍

Comment thread test/testerrorlogger.cpp Outdated
suppressions.clear();
suppressions.emplace_back("abc", "a.c", 10U);
suppressions.emplace_back("unmatchedSuppression", "*", 0U);
suppressions.emplace_back("unmatchedSuppression", "*", /*Suppressions::Suppression::NO_LINE*/-1); //TODO find out why clang can't link with Suppressions::Suppression::NO_LINE
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem you had was a linker error because there is no definition for Suppressions::Suppression::NO_LINE.

I would guess that if you replace the variable with such constant it will work:

enum {NO_LINE=-1};

Another solution is to add a variable definition.. but that is unfortunate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well the constant is defined in suppression.h which is included in testerrorlogger.cpp. adding a new constant with the same value just for the purpose of making clang happy is a bit over the top in my opinion, other linkers (VC and GCC) didn't complain :/

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want that you replace the variable declaration in suppression.h

This line in suppressions.h:

static const int NO_LINE = -1;

Write this instead:

enum { NO_LINE = -1 };

other linkers (VC and GCC) didn't complain

In my humble opinion that is strange. Maybe I am wrong, but the code is not valid as far as I see.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I read it on this page:
https://en.cppreference.com/w/cpp/language/static

the code is not valid. you need to define Suppressions::Suppression::NO_LINE also.

Comment thread lib/cppcheck.h Outdated
* and if it's possible at all */
bool isUnusedFunctionCheckEnabled() const;

/**
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test is rewritten as I suggest I assume you can revert these changes.

@PKEuS PKEuS merged commit 83cb0b3 into cppcheck-opensource:master Sep 18, 2018
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