findtoken.h: make sure code is being matchcompiled #6641

firewave · 2024-07-26T06:48:01Z

No description provided.

firewave · 2024-07-26T06:52:58Z

-D __GNUC__ --check-level=exhaustive lib/utils.cpp

Clang 18 851,620,331 -> 809,036,639
GCC 14 880,259,918 -> 841,976,696

This should help with https://trac.cppcheck.net/ticket/12271.

Detecting match calls in headers is tracked in https://trac.cppcheck.net/ticket/11541.

This is obviously extremely horrible and needs to be done differently. Because templates are involved I could not come up with something better for now - but because of the sizable impact I published it anyways.

firewave · 2024-07-29T16:38:49Z

The example from https://trac.cppcheck.net/ticket/10765#comment:4.

Clang 17 8,156,089,970 -> 7,669,984,764

firewave · 2024-07-29T18:50:02Z

@pfultz2 Care to have a look at this? Thanks!

firewave · 2024-08-28T10:27:28Z

The example from https://trac.cppcheck.net/ticket/10765#comment:4.

Clang 17 3,753,065,367 -> 3,237,960,965

This makes a lot of difference and we really need to figure out how to get this change in properly.

pfultz2 · 2024-09-07T20:41:18Z

lib/findtoken.h

        }
-        if (Token::Match(tok, "if|for|while (") && Token::simpleMatch(tok->linkAt(1), ") {")) {
+        //if (Token::Match(tok, "if|for|while (") && Token::simpleMatch(tok->linkAt(1), ") {")) {
+        if (findTokensSkipDeadCodeImplMatch_1(tok)) {


Why cant the match compiler be updated to automatically generate this? This seems very problematic.

Seems obvious in hindsight. But thinking about it I think there might be problems.

That would cause the matchcompiler.h include to spill which seems undesired. Same with the match functions. The latter will probably cause conflicts in other matchcompiled files which include this.

Can check thought if if it would work if we also process header files. Would be a better approach - albeit I would prefer if it only were temporary.

That would cause the matchcompiler.h include to spill which seems undesired

What do you mean spill? There is a header guard.

Same with the match functions. The latter will probably cause conflicts in other matchcompiled files which include this.

I thought each function is appended a unique number.

That would cause the matchcompiler.h include to spill which seems undesired

What do you mean spill? There is a header guard.

I mean that it will be includes in all files including this even when not necessary. It is just not clean.

Same with the match functions. The latter will probably cause conflicts in other matchcompiled files which include this.

I thought each function is appended a unique number.

I am not sure if that is unique globally or locally. The first is unnecessary since we only generate functions within source files. But will immediately come up if I try to include the headers.

Okay. I just remembered the main issue.

The files need to be matchcompiled thus you need to store the modified files and build those. But if you do that with headers you also need to inject an additional header path in release builds and you two headers of the same name which might also cause issues.

So it is not just include the *.h in the matchcompiler.

We can move this header to another directory thats not in the include path. For matchcompiler builds we can add the include path to the directory where these files are dumped, and without matchcompiler we can just add the include path to the original header.

We have no "other directories". We are using a flat structure and introducing an additional folder (for a single file mind you) will have a ripple effect. This needs to be addressed in a different way for each build system and I have the feeling that would not work at all for the Visual Studio release build we perform in the CI.

I tried introducing support for subfolders for the sources in the matchcompiler at some point. It was a pain, very hacky and I never got it to work fully so I abandoned it. I did that when I proposed the splitting of valueflow.cpp.

On a side note. I just realized that the matchcompiler is not integrated at all in the Visual Studio project file we are providing so people using that to build their local binary will have a much slower executable. Another reason to dump these files and go with CMake all the way.

We have no "other directories". We are using a flat structure and introducing an additional folder (for a single file mind you) will have a ripple effect. This needs to be addressed in a different way for each build system and I have the feeling that would not work at all for the Visual Studio release build we perform in the CI.

We can always create a directory. Then we can get it running using cmake. For the other build systems, its just a matter of adding an include path. There wont be a perf regression for these other build systems because they are not using the matchcompiler on header files in the first place. Users of these build systems can provide the support to improve it. We shouldn't make it a barrier for improving code quality.

Also, it is a single file, but this will make it easier to add more header-only files in the future.

Another reason to dump these files and go with CMake all the way.

I agree. CMake already has a lot of enhancements already over the other builds such as parallel test running, and this will be another additional enhancement.

firewave · 2024-12-14T13:49:10Z

I do not want the changes to go in this way but I am marking this for review so I hopefully get some more feedback on how to resolve this.

danmar · 2024-12-15T10:20:30Z

lib/findtoken.h

    return result;
 }

+bool findTokensSkipDeadCodeImplMatch_1(const Token* tok);


I think it's unfortunate to separate the match patterns from the code. It can lead to bugs. Is it possible to move the whole function implementation to the cpp file..

I think it's unfortunate to separate the match patterns from the code. It can lead to bugs.

Yeah - not my intention at all to get this code in. As I wrote I want to get help on how to get this in properly.

Is it possible to move the whole function implementation to the cpp file..

Since templates are involved that is problematic. I failed in my attempts so far.

hmm.. I haven't tried to take this the whole way but I have the idea we could do something like this. Move the findTokensSkipDeadCodeImpl template to the findtoken.cpp file.

In findtoken.h:

std::vector<Token*> findTokensSkipDeadCodeCpp(const Library& library, Token* start, const Token* end, const std::function<bool(const Token*)>& pred, const std::function<std::vector<MathLib::bigint>(const Token*)>& evaluate); std::vector<const Token*> findTokensSkipDeadCodeCpp(const Library& library, const Token* start, const Token* end, const std::function<bool(const Token*)>& pred, const std::function<std::vector<MathLib::bigint>(const Token*)>& evaluate); template<class T, class Predicate, class Evaluate, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )> std::vector<T*> findTokensSkipDeadCode(const Library& library, T* start, const Token* end, const Predicate& pred, const Evaluate& evaluate) { std::function<bool(const Token*)> predFunc = pred; std::function<std::vector<MathLib::bigint>(const Token*)> evaluateFunc = evaluate; return findTokensSkipDeadCodeCpp(library, start, end, predFunc, evaluateFunc); }

The findTokensSkipDeadCodeCpp are defined in the findtoken.cpp and invokes the findTokensSkipDeadCodeImpl.

The code does get more messy as there are "Cpp" intermediate functions..

I have a solution which builds. It is a bit messier than the hack in terms of total changes. I will give it a spin and update the PR later.

Done. It is even a tad faster than the previous hack. Will post profiling numbers later.

firewave · 2024-12-22T17:16:39Z

-D __GNUC__ --check-level=exhaustive lib/utils.cpp

Clang 19 699,175,113 -> 652,340,049
GCC 14 744,553,244 -> 702,309,116

The example from https://trac.cppcheck.net/ticket/10765#comment:4.

Clang 19 3,624,176,882 -> 3,063,883,851
GCC 14 3,884,644,414 -> 3,374,187,145

callgrind in the CI
63,702,930,187 -> 63,303,081,301

danmar · 2025-01-01T22:14:08Z

nice speedups! I am curious.. do you have any theory why there is speedup it just looks like a refactoring to me?

firewave · 2025-01-01T22:20:05Z

nice speedups! I am curious.. do you have any theory why there is speedup it just looks like a refactoring to me?

The Token::Match() calls were not matchcompiled because they lived in a header. See previous discussion here.

danmar · 2025-01-01T22:23:39Z

The Token::Match() calls were not matchcompiled because they lived in a header. See previous discussion here.

ah yes! 👍

firewave · 2025-01-04T18:37:09Z

@pfultz2 does this look okay to you?

pfultz2 · 2025-01-04T18:54:02Z

lib/findtoken.h

+                                    const std::function<bool(const Token *)> &pred,
+                                    const std::function<bool(T *)>& found,
+                                    const std::function<std::vector<MathLib::bigint>(const Token *)> &evaluate,
+                                    bool skipUnevaluated) = delete;


Why does this need to be deleted?

Because we are only using specializations. If this is not deleted the compiler will complain about it missing.

Ah, I see. Using templates makes no sense anymore everything related to it can just be dropped.

pfultz2 · 2025-01-04T18:55:51Z

lib/findtoken.h

+                                    const std::function<std::vector<MathLib::bigint>(const Token *)> &evaluate,
+                                    bool skipUnevaluated) = delete;
+
+    template<>


This line can be removed.

This and the others are template specializations. I thought those needed to be indicated as templates as well.

Yea its simpler to treat it as function overloading.

Too many trees ...

pfultz2 · 2025-01-04T18:56:01Z

lib/findtoken.h

+                                    const std::function<std::vector<MathLib::bigint>(const Token *)> &evaluate,
+                                    bool skipUnevaluated);
+
+    template<>


This line can be removed.

pfultz2 · 2025-01-04T18:57:03Z

lib/findtoken.h

+    std::function<bool(T*)> f = [&](T* tok) {
+        result.push_back(tok);
+        return false;
+    };


Why cant this be defined inline?

Ask the compiler. It cannot convert the lambda to std::function if it is inline.

In file included from /home/user/CLionProjects/cppcheck/lib/valueflow.cpp:87: /home/user/CLionProjects/cppcheck/lib/findtoken.h:114:11: error: no matching function for call to 'findTokensSkipDeadCodeImpl' 114 | (void)internal::findTokensSkipDeadCodeImpl( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/user/CLionProjects/cppcheck/lib/findtoken.h:131:12: note: in instantiation of function template specialization 'findTokensSkipDeadCode<Token, (lambda at /home/user/CLionProjects/cppcheck/lib/valueflow.cpp:5826:67), std::vector<long long> (*)(const Token *), void>' requested here 131 | return findTokensSkipDeadCode(library, start, end, pred, &evaluateKnownValues); | ^ /home/user/CLionProjects/cppcheck/lib/valueflow.cpp:5826:12: note: in instantiation of function template specialization 'findTokensSkipDeadCode<Token, (lambda at /home/user/CLionProjects/cppcheck/lib/valueflow.cpp:5826:67), void>' requested here 5826 | return findTokensSkipDeadCode(library, start, scope->bodyEnd, [&](const Token* tok) { | ^ /home/user/CLionProjects/cppcheck/lib/findtoken.h:79:10: note: candidate template ignored: could not match 'std::function<bool (T *)>' against '(lambda at /home/user/CLionProjects/cppcheck/lib/findtoken.h:119:9)' 79 | bool findTokensSkipDeadCodeImpl(const Library &library, | ^

Ah - without the template stuff that now works again.

pfultz2 · 2025-01-04T18:58:53Z

lib/findtoken.cpp

+#include "token.h"
+
+template<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
+static bool findTokensSkipDeadCodeImpl2(const Library& library,


How about we name this findTokensSkipDeadCodeImplGeneric instead?

Agreed. I forgot to give this a proper name.

pfultz2 · 2025-01-04T18:59:23Z

lib/findtoken.cpp

+}
+
+namespace internal {
+    template<>


This line can be removed.

pfultz2 · 2025-01-04T18:59:28Z

lib/findtoken.cpp

+        return findTokensSkipDeadCodeImpl2(library, start, end, pred, found, evaluate, skipUnevaluated);
+    }
+
+    template<>


This line can be removed.

pfultz2 · 2025-01-04T19:02:21Z

lib/astutils.cpp

        const Token* operator()(const Token* start, const Token* end, F f) const
        {
-            return findToken(start, end, f);
+            return findToken(start, end, std::move(f));


The predicate is taken as const ref, so this will cause an extra copy.

Good catch. Leftover from some wrongly applied changes.

I wonder why clang-tidy did not report this. Possibly because it is not yet a std::function and still a function pointer. Something to look into.

firewave · 2025-01-04T19:44:46Z

Thanks. I think I addressed all of it.

firewave force-pushed the pm-findtoken branch from ba97b80 to 042c118 Compare August 28, 2024 23:24

firewave mentioned this pull request Sep 2, 2024

ValueFlow: various small optimizations #6756

Draft

pfultz2 reviewed Sep 7, 2024

View reviewed changes

firewave force-pushed the pm-findtoken branch from 042c118 to aa64d95 Compare December 14, 2024 13:48

firewave marked this pull request as ready for review December 14, 2024 13:49

danmar reviewed Dec 15, 2024

View reviewed changes

firewave force-pushed the pm-findtoken branch 3 times, most recently from 8a467bc to 4eb26f1 Compare December 22, 2024 14:46

firewave force-pushed the pm-findtoken branch from 4eb26f1 to 2b557f5 Compare December 24, 2024 21:37

danmar approved these changes Jan 1, 2025

View reviewed changes

pfultz2 reviewed Jan 4, 2025

View reviewed changes

pred

7abea40

firewave added 8 commits January 4, 2025 20:34

move

08723e6

eval

c107d5f

found

10f1de0

cpp

a6e0edb

restore

2d8b7e3

internal

f17db22

const ref

02e7200

no template

20e8ab3

firewave force-pushed the pm-findtoken branch from 2b557f5 to 6d67539 Compare January 4, 2025 19:43

found

7878241

firewave force-pushed the pm-findtoken branch from 6d67539 to 7878241 Compare January 4, 2025 19:47

pfultz2 approved these changes Jan 4, 2025

View reviewed changes

firewave merged commit 8b764d8 into danmar:main Jan 4, 2025
60 checks passed

firewave deleted the pm-findtoken branch January 4, 2025 23:58

findtoken.h: make sure code is being matchcompiled #6641

findtoken.h: make sure code is being matchcompiled #6641

Uh oh!

Conversation

firewave commented Jul 26, 2024

Uh oh!

firewave commented Jul 26, 2024

Uh oh!

firewave commented Jul 29, 2024

Uh oh!

firewave commented Jul 29, 2024

Uh oh!

firewave commented Aug 28, 2024

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 Dec 14, 2024

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 Dec 22, 2024

Uh oh!

danmar commented Jan 1, 2025

Uh oh!

firewave commented Jan 1, 2025

Uh oh!

danmar commented Jan 1, 2025

Uh oh!

firewave commented Jan 4, 2025

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!

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!