Fix #11483 FN unusedFunction for method with inline implementation by chrchr-github · Pull Request #4692 · cppcheck-opensource/cppcheck

chrchr-github · 2023-01-08T00:29:59Z

No description provided.

firewave · 2023-01-08T14:28:02Z

        return _next;
    }

-    /** IntValue interprets the attribute as an integer, and returns the value.


Please do not change external files. Put it in the suppression file.

firewave · 2023-01-08T14:28:54Z

    }

    /** get allocation id for function by name (deprecated, use other alloc) */
+    // cppcheck-suppress unusedFunction - only used in unit tests


I would drop the mention of unit tests for obvious helper functions like this. Same for the other methods in this file.

I will review this in more detail later on though.

firewave · 2023-01-08T14:29:20Z

        return const_cast<Scope *>(this->findScope(tok, const_cast<const Scope *>(startScope)));
    }

-    bool isVarId(nonneg int varid) const {


We should keep this for now.

firewave · 2023-01-08T14:29:49Z

    void isSigned(const bool sign) {
        setFlag(fIsSigned, sign);
    }
-    bool isPointerCompare() const {


We should keep this even if it isn't used. Same for the other methods in this file.

firewave · 2023-01-08T14:32:08Z

        if: false # TODO: fails with preprocessorErrorDirective - see #10667
        run: |
-          ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__CPPCHECK__ -D__GNUC__ -DQT_VERSION=0x050000 -DQ_MOC_OUTPUT_REVISION=67 --enable=unusedFunction --exception-handling -rp=. --project=cmake.output/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr
+          ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__CPPCHECK__ -D__GNUC__ -DQT_VERSION=0x050000 -DQ_MOC_OUTPUT_REVISION=67 --enable=unusedFunction --exception-handling -rp=. --project=cmake.output/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr -icmake.output.notest


We should not ignore the generated files.

This will also lead to false positives since function declarations are missing.

As long as suppressions don't work, this is dead in the water anyway. The problem seems to be related to the way included code is handled.

Ah - this is causing the "false positives" in the GUI code. Please revert this.

firewave · 2023-01-09T14:40:28Z

No need to close this. The change is good.

There's several issues with suppressions. Some of them also keeping me from finishing some PRs.

chrchr-github · 2023-01-09T15:55:07Z

Uhmm, I see no obvious way to fix this. The warning is produced for a location in the .cpp file, whereas the suppression is in the header file. All tokens from the included file are assigned the same line in the .cpp, so that their actual origin is lost.

firewave · 2023-01-10T12:53:44Z

I added a test for this case here: firewave@2d8d48c. The location is wrong when the fix is applied. So this might be a place to start at.

firewave · 2023-03-04T18:23:44Z

I want to finish up with my suppression and platform stuff first and will take a look afterwards. I have a few things to look at in simplecpp anyways and will do it along the way.

firewave · 2023-04-23T22:46:33Z

Uhmm, I see no obvious way to fix this. The warning is produced for a location in the .cpp file, whereas the suppression is in the header file. All tokens from the included file are assigned the same line in the .cpp, so that their actual origin is lost.

This is not a simplecpp issue it seems.

The line is actually correct just the filename isn't. The problem is there is no file context for the token and we just use the source filename from the TokenList without even considering the fileIndex.

We probably need to add all include files to the TokenList.mFiles (there's mOrigFiles which should indicate we did not supply that as input) and store the proper file index in the Token.

There might be a different way though as we do report other issues from headers with the proper filename.

I updated the branch with that test: https://github.com/firewave/cppcheck/commits/inc-test

firewave · 2023-04-25T21:43:04Z

There's TokenList::file() as convenience function. Didn't know about that until yesterday,

Yeah, me neither.
So just using the correct filename fixes the problem with the location. For some reason, I thought the index was also necessary.

firewave · 2023-04-25T22:52:21Z

Please add a test and look at the open conversations.

We should suppress the warning for all externals.

I think the GUI warnings with the overrides might be false positives maybe related to something I already reported. I will take a look later. The other one is related to commented code and should be commented instead of being removed - see #4688.

chrchr-github · 2023-04-26T10:18:05Z

Please feel free to take over. You already found the solution, and I'm kinda tired of messing with this.

firewave · 2023-04-26T11:09:56Z

I just gave a few pointers so this could advance. This is probably far from a solution as during debugging I did not see more than a single file ever being stored in the TokenList and it seems the only fileIndex every used was 0. So it appears the functionality is there but all the hookups are missing. I have no idea what kind of ripple effect this might have.

And imagine how tired I am that I have to wait weeks/months for things to be reviewed at all...and the countless conflicts I have to resolve all the time because of that. And I already stated that I am not able to look into things beyond the one I am still working on, sorry.

firewave · 2023-07-27T20:53:32Z

I think I figured out the problem - at least within the unit test. The fileIndex is correct in the simplecpp::TokenList but then we get the processed code as a whole and that lacks any information about the file and we pass that to the analysis in the test. So it could just be an issues in the unit tests. The preprocessing in the tests is also not the same as in the production code.

firewave · 2023-07-31T14:15:44Z

The tests in #5281 show that the suppressions are working as expected. The failure in the unit tests is caused by the way we preprocess code for them.

chrchr-github added 3 commits January 7, 2023 22:29

Fix #11483 FN unusedFunction for method with inline implementation

6b6eb0e

Remove unused functions, add suppressions, exclude autogen dir

d2fe8a1

Format

b375eb3

chrchr-github marked this pull request as draft January 8, 2023 00:30

chrchr-github added 2 commits January 8, 2023 01:36

Remove unused member

3b55338

Fix comment, exclusion

fe2e2c7

firewave requested changes Jan 8, 2023

View reviewed changes

chrchr-github closed this Jan 9, 2023

firewave reopened this Jan 9, 2023

chrchr-github added 2 commits April 25, 2023 23:34

Fix #11483

5ee948e

Merge

1478626

firewave reviewed Apr 25, 2023

View reviewed changes

chrchr-github added 4 commits April 25, 2023 23:49

Fix merge

53bd055

Fix merge

51fe6b5

Use helper

402326a

Undo

a890bca

chrchr-github closed this Apr 26, 2023

chrchr-github deleted the chr_Fix11483 branch May 22, 2023 19:02

firewave mentioned this pull request Jul 31, 2023

added (TODO) test cases for #11483 #5281

Closed

firewave mentioned this pull request Sep 18, 2023

fixed #11483 (FN unusedFunction for method with inline implementation) #5457

Merged

Conversation

chrchr-github commented Jan 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Jan 9, 2023

Uh oh!

chrchr-github commented Jan 9, 2023

Uh oh!

firewave commented Jan 10, 2023

Uh oh!

firewave commented Mar 4, 2023

Uh oh!

firewave commented Apr 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Apr 25, 2023

Uh oh!

chrchr-github commented Apr 26, 2023

Uh oh!

firewave commented Apr 26, 2023

Uh oh!

firewave commented Jul 27, 2023

Uh oh!

firewave commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants