Added rule to simplify token matching #6472
Conversation
|
We should integrate this into the selfcheck. And we should also evaluate if parts of the internal check could be implemented using rules instead. A Python test for this file so it works as expected would also be great. |
|
Applying these changes might incur a minor performance hit as it performs additional checks. Also the code is probably not being inlined as the implementation is inside the source. |
|
We might also change |
Good idea, added this rule in accordance. |
This rule provides a bunch of results. I'll provide a seperate PR (#6476) to fix them. The interesting part, is if performance is affected by these changes. In general, it makes the code more readable. These are the findings ` |
|
I would prefer if we would add the existing rules to the selfcheck so we have a baseline and enforcement and then add the new rules along with the fixes in one PR so we have that properly associated. |
|
Aren't these rules in the selfcheck already? |
No, that file is unused. As mentioned in my previous comment we have the |
|
I would also like to profile the impact of the changes first. It should be miniscule but just to be sure. I am not sure if I still get around to it today. |
groan
We already have similar simplification rules in place anyway. |
I assume |
|
thanks.. my problem with this is that I don't like the rule files and would rather like to get rid of this old mistake. It's not hard to implement it with an addon instead. |
|
IIRC @pfultz2 is/was using rule files at some point. |
Are you suggesting to drop the feature as a whole? |
yes personally I would like to drop this feature. I believe it is deprecated since about 10-15 years. |
There are cases where PCRE Rules are still helpful, for example to enforce some very simple rules, such as forbidding special keyword usage like goto. IMHO: There is no need to deprecate this feature, one simply has to know when it is better to either implement a PCRE Rule or an addon or even a Cppcheck. |
Performance should be better and can probably still be improved. If you have an example I can take a look. |
If we could at least get rid of that stupid PCRE dependency that would be a major step forward.
I would expect that an addon that search for goto would be very fast. And addons does not have to be written with Python although we don't have ready-made open source code to import the dumpdata in any other language. Compiled languages can be used. |
#6211 at least tries to encapsulate it so it could be replaced with something else. |
👍 I don't know the exact difference of PCRE and C++ regex but I would imagine that it's only different in some edge cases. Getting rid of PCRE dependency would be a big relief. |
PCRE is super fast and |
Yes |
Its really easy to write an addon for such a check(and this is the entire python file): @cppcheck.checker
def GotoStatement(cfg, data):
for token in cfg.tokenlist:
if token.str != 'goto':
continue
cppcheck.reportError(token, "style", "Goto considered harmful.")Where I still use the rules, its actually for cases where I have a more complicated regex(such as Also, the PCRE rules run much faster than an addon(loading the xml in python is much slower than running a regex directly). |
I would understand that argument if it was executed directly on the file. I would assume that std::regex finish in a fraction of the time it takes for cppcheck to parse the file and to perform its valueflow. |
that looks like a case that would benefit to be implemented in python instead :-) as the python script could be flexible. and I assume more readable. I don't see what the regexp does actually. :-( |
|
Closing as the file in question has been removed in #6951. IIRC the internal check was also improved to detect this. If there is still something to be improved on, please file a ticket so it is tracked. |
This rule catches cases where multiple
tok->next()-next()calls can be simplified totok-tokAt(2)Reference: https://github.com/danmar/cppcheck/pull/6442/files/13b2cabb6384ff5279b17a75062a63c5f240f74a#r1623220527