Fix unmatched suppression (#5704)#3886
Conversation
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
I think the approach seems reasonable overall. 👍
I am a bit skeptic technically about including the whole tokenizer in suppressions. It's a heavy dependency. I guess it could just settle with token.h.
| } | ||
|
|
||
| /** | ||
| * This function require that both tokens and suppressions are sorted in line |
There was a problem hiding this comment.
That assumption will not work when files are included.
There was a problem hiding this comment.
OK? You mean that both suppression and token list can be multiple files in some 'list'??
There was a problem hiding this comment.
As far as I know the suppressions are listed in arbitrary order. If you want to sort them feel free to do it.
The tokenizer.tokens() contains all tokens in a translation unit.
If you have a file foo.h with 100 lines of code.. then you put this code in foo.c:
#include "foo.h"
int x;
then the tokenizer.tokens() will first contain the tokens from "foo.h" and then the "int x;" tokens.. so if you just look at the line number it will decrease when the "int x;" code is reached.
|
|
||
| #include "config.h" | ||
| #include "errortypes.h" | ||
| #include "tokenize.h" |
There was a problem hiding this comment.
it does not seem this include is necessary here. I would prefer a forward declaration in the header:
class Tokenizer;
There was a problem hiding this comment.
Yes, and then include in the .cpp file? Or should we just only take a pointer to the first token? I just looked at some other functions that used the tokenizer and then took the token list from there.
There was a problem hiding this comment.
hmm.. I don't have a super strong opinion if you should pass Tokenizer or Token. Both will work fine. Well.. if you pass tokenizer it's more apparent that the whole token list is used. so feel free to keep the interface as is.
|
Another thing. I do not really see what in that scriptcheck goes wrong. Am I blind or ... ? |
I think the diff tells you what to change in |
|
Anyway, I have written a new batch of test-files to trigger all the things discussed in this PR. After I have something new we could talk about making a new PR or post in this one. |
|
I have a new loop to mark suppression's as checked. I looks good. But I do not know how slow it is going to be. On my tests it works fine, in project mode, on a directory or on a specific file. |
| /** | ||
| * This function require that both tokens and suppressions are sorted in line | ||
| * number order. | ||
| * Loop on mSuppressions over and oer again might not be so fast. |
There was a problem hiding this comment.
The placement here indicates that this comment will describe what the function is doing. But the text does not say that.
It feels more like some TODO comment to speedup..
I don't feel very worried that this function will be a bottleneck since it's only called once on each file configuration right? But I suggest that you write TODO. maybe put it inside the function body to indicate that it doesn't describe the function.
| currLineNr = tok->linenr(); | ||
| currFileIdx = tok->fileIndex(); | ||
| for (auto &supp : mSuppressions) { | ||
| if (supp.fileName == tokenizer.list.file(tok) && supp.lineNumber == currLineNr) { |
There was a problem hiding this comment.
Maybe you can put the lineNumber comparison before the filename comparison. A integer comparison should be faster than a string comparison.
There was a problem hiding this comment.
It is run one time per configuration for every file. Even if hash would skip other checks. I put it before that check cause I had problem with some suppression's not getting their check flag set. But that is mostly because putting 'same' test in different ifdef combination. In the 'real' world that maybe not happening so often.
Also, maybe some debug flag should print suppression's that does not have the checked flag set.
| void inlinesuppress_unusedFunction() const { // #4210, #4946 - wrong report of "unmatchedSuppression" for "unusedFunction" | ||
| Suppressions suppressions; | ||
| suppressions.addSuppression(Suppressions::Suppression("unusedFunction", "test.c", 3)); | ||
| auto spr = Suppressions::Suppression("unusedFunction", "test.c", 3); |
There was a problem hiding this comment.
generally.. I think it's better to write out the full name. suppression is better that spr unless it's an abbreviation used everywhere..
|
Question, how much do save by calculating the checksum and then skipping some configurations cause of same checksum?? I put the markUnmatchedInlineSuppressionsAsChecked() call before the check to ensure all relevant suppression's get checked. But maybe I am to cautious from having constructed a very strange test to trigger things. |
When incremental analysis is used.. for an unchanged code it can save ~ 98% of time or something like that. The calculation is quick. Analysis is very slow. |
|
sorry there is some conflict now.. could you look at that. |
The conflict is the Makefile generated by dmake. But if I do not generate that file some CI will fail. I will put up my latest version. The reason why I talk about the checksum is that at the moment the check if suppression is being checked is done before. If we do after some of the work will already be done by the normal checks. The thing is just if we should have some extra check available to report if there are suppression's that are not marked as checked. |
Yes that sounds reasonable. To avoid that unused inline suppressions are left by mistake in the code. |
|
I merged your changes so far. feel free to look into the unmatchedsuppression. |
3 participants
I hope this is an OK way to solve it. The problem with adding this flag is that you more need to do full flow to get the result you want from functions like Suppressions::getUnmatchedLocalSuppressions().