Revert valueflow split #6991

pfultz2 · 2024-11-05T16:00:24Z

Revert most of the moving passes out of valueflow.cpp. Splitting up valueflow should be organized differently.

…rate file (danmar#6845)" This reverts commit 66d53a2.

…e file (danmar#6783)" This reverts commit c348128.

…e file (danmar#6782)" This reverts commit a63b165.

…ar#6617)" This reverts commit c1cd1e2.

…)" This reverts commit 25d7921.

pfultz2 · 2024-11-08T20:45:34Z

The error in CI is unrelated to this PR, and is a failure with getting qt packages on windows:

INFO    : aqtinstall(aqt) v3.1.18 on Python 3.11.9 [CPython MSC v.1938 64 bit (AMD64)]
WARNING : Specified Qt version "6.7.3" did not exist when this version of aqtinstall was released. This may not install properly, but we will try our best.
WARNING : Specified target combination "windows desktop win64_msvc2022_64" did not exist when this version of aqtinstall was released. This may not install properly, but we will try our best.
ERROR   : The packages ['qt_base', 'qtcharts'] were not found while parsing XML of package information!
==============================Suggested follow-up:==============================
* Please use 'aqt list-qt windows desktop --arch 6.7.3' to show architectures available.
* Please use 'aqt list-qt windows desktop --modules 6.7.3 <arch>' to show modules available.

pfultz2 · 2024-11-09T16:56:55Z

This same failure is on the main branch.

firewave · 2024-11-16T14:56:53Z

Just looking at the changes files and not the changes it self.

Please revert the *.ts changes.

It also seems you missed to merge back vf_number.cpp and vf_enumvalue.cpp.

firewave · 2024-11-16T14:59:14Z

I also think the infer.cpp changes are fine and should stay.

firewave · 2024-11-16T15:02:54Z

vf_debug.cpp also looks like it should be merged back.

danmar · 2024-11-17T19:50:16Z

Please revert the *.ts changes.

imho the ts files should not be changed behind our backs.

I have always updated the ts files during the release. How do I update the ts files nowadays when qmake support is removed? It would be good to document that step in the createrelease document which I follow when I make the releases.

danmar · 2024-11-17T19:56:33Z

Please revert the *.ts changes.

I created https://trac.cppcheck.net/ticket/13322

firewave · 2024-11-17T21:03:24Z

Please revert the *.ts changes.

imho the ts files should not be changed behind our backs.

I have always updated the ts files during the release. How do I update the ts files nowadays when qmake support is removed? It would be good to document that step in the createrelease document which I follow when I make the releases.

That is done implicitly. Only updating them on release never made sense because that means that they will always be out-of-date during development and might render translated versions unusable.

danmar · 2024-11-18T11:59:23Z

That is done implicitly. Only updating them on release never made sense because that means that they will always be out-of-date during development and might render translated versions unusable.

ok well if you can make it work properly so that they are only updated when I change a related file I am not against it. Paul did not change any related files here in this PR.

pfultz2 · 2024-11-18T15:36:24Z

Please revert the *.ts changes.

I am not sure what you are talking about. I reverted the actual commits which should revert everything.

pfultz2 · 2024-11-18T15:39:22Z

It also seems you missed to merge back vf_number.cpp and vf_enumvalue.cpp.

I'll address that in a leter PR.

I also think the infer.cpp changes are fine and should stay.

Let me put those changes back in this PR.

vf_debug.cpp also looks like it should be merged back.

I dont think the pass is significant enough to have in a seperate file.

firewave · 2024-11-20T12:31:10Z

It also seems you missed to merge back vf_number.cpp and vf_enumvalue.cpp.

I'll address that in a leter PR.

I would prefer if we could revert it all in a single PR.

vf_debug.cpp also looks like it should be merged back.

I dont think the pass is significant enough to have in a seperate file.

That's what I said.

pfultz2 · 2024-11-20T17:58:58Z

I would prefer if we could revert it all in a single PR.

I would prefer it be merged in quicker so we can avoid major merge conflicts if there is another PR making tweaks to ValueFlow.

pfultz2 · 2024-11-20T17:59:57Z

vf_debug.cpp also looks like it should be merged back.

I dont think the pass is significant enough to have in a seperate file.

That's what I said.

I see what you are saying now, I'll address that in a follow-up PR.

…into revert-valueflow-split

pfultz2 · 2024-11-20T18:18:37Z

I also think the infer.cpp changes are fine and should stay.

I reverted those now.

firewave · 2024-11-20T18:35:00Z

I would prefer it be merged in quicker so we can avoid major merge conflicts if there is another PR making tweaks to ValueFlow.

Yeah - I refrained from publishing ones affecting those. Will take a look later today and will merge if it looks okay. I can also do the remaining reverting immediately after.

firewave · 2024-11-21T13:33:30Z

I was too tired last night - will take a look when I get back in later. Sorry about that.

firewave · 2024-11-21T16:07:00Z

lib/valueflow.cpp

@@ -4571,6 +6163,55 @@ static void valueFlowSmartPointer(TokenList &tokenlist, ErrorLogger & errorLogge
    }
 }

+static Library::Container::Yield findIteratorYield(Token* tok, const Token** ftok, const Settings& settings)


The changes from https://github.com/danmar/cppcheck/pull/6737/files#diff-4ea309cd6baf7f501ec7f0a87618c38284008fc3d4579539c8b0d792e37c4765 are not included in this.

firewave · 2024-11-21T16:09:05Z

lib/valueflow.cpp

        VFA_CPP(valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings, skippedFunctions)),
        VFA(valueFlowSafeFunctions(tokenlist, symboldatabase, errorLogger, settings)),
    });

    runner.run_once({
        VFA(valueFlowDynamicBufferSize(tokenlist, symboldatabase, errorLogger, settings)),
-        VFA(analyzeDebug(tokenlist, errorLogger, settings)), // TODO: add option to print it after each step/iteration
+        VFA(valueFlowDebug(tokenlist, errorLogger, settings)),


The change from 0cf639c#diff-16a9ec3ff1314646e106eb413234f5614e3a7f527fb82d0a369755bee65a7e31 is lost.

You mean the comment?

Yes. Nothing to be addressed here. Just to note the differences I spotted during the review.

firewave

The *.ts file changes still need to be dropped.

Otherwise it looks fine. Fortunately not many changes (only two commits - unless I missed something) were done to the split out files.

firewave · 2024-11-21T19:03:23Z

Just drop the .ts changes and we can merge it. The rest can be addressed afterwards.

firewave

I pushed a commit dropping the .ts changes.

restores the changes lost in #6991

pfultz2 added 13 commits November 5, 2024 09:41

Revert

22bfe50

Revert "ValueFlow: extracted valueFlowSymbolicOperators() into sepa…

0998cc0

…rate file (danmar#6845)" This reverts commit 66d53a2.

Revert

adbab16

Revert "ValueFlow: extracted valueFlowSwitchVariable() into separat…

feda07f

…e file (danmar#6783)" This reverts commit c348128.

Revert "ValueFlow: extracted valueFlowInferCondition() into separat…

c869d78

…e file (danmar#6782)" This reverts commit a63b165.

Revert "ValueFlow: extracted even more code into separate files (danm…

2b718a1

…ar#6617)" This reverts commit c1cd1e2.

Revert "ValueFlow: extracted more code into separate files (danmar#6449…

4d64677

…)" This reverts commit 25d7921.

Update

b3d5939

Format

235e12f

Remove lib.pri file

b73913f

Remove vf_iterators

878ac2f

Remove vf_bailout

3928d1f

Merge branch 'main' into revert-valueflow-split

8534fec

Merge branch 'main' into revert-valueflow-split

9e3c543

pfultz2 mentioned this pull request Nov 13, 2024

ValueFlow: extracted valueflowForward*() helpers into a separate file #6947

Closed

Update

6b3680b

pfultz2 added 3 commits November 20, 2024 12:01

Merge branch 'main' into revert-valueflow-split

3ec1753

Move infer function

b4f626d

Merge branch 'revert-valueflow-split' of github.com:pfultz2/cppcheck …

369fc3e

…into revert-valueflow-split

danmar approved these changes Nov 20, 2024

View reviewed changes

firewave reviewed Nov 21, 2024

View reviewed changes

firewave requested changes Nov 21, 2024

View reviewed changes

revert .ts changes

4fa62cd

firewave approved these changes Nov 22, 2024

View reviewed changes

firewave merged commit 9989e18 into danmar:main Nov 22, 2024
60 checks passed

firewave mentioned this pull request Nov 22, 2024

restored ValueFlow changes #7032

Merged

pfultz2 deleted the revert-valueflow-split branch November 22, 2024 14:35

chrchr-github pushed a commit that referenced this pull request Nov 23, 2024

restored ValueFlow changes (#7032)

09b5f4d

restores the changes lost in #6991

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert valueflow split #6991

Revert valueflow split #6991

pfultz2 commented Nov 5, 2024

pfultz2 commented Nov 8, 2024

pfultz2 commented Nov 9, 2024

firewave commented Nov 16, 2024 •

edited

Loading

firewave commented Nov 16, 2024

firewave commented Nov 16, 2024

danmar commented Nov 17, 2024 •

edited

Loading

danmar commented Nov 17, 2024 •

edited

Loading

firewave commented Nov 17, 2024

danmar commented Nov 18, 2024 •

edited

Loading

pfultz2 commented Nov 18, 2024

pfultz2 commented Nov 18, 2024

firewave commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

firewave commented Nov 20, 2024

firewave commented Nov 21, 2024

firewave Nov 21, 2024 •

edited

Loading

firewave Nov 21, 2024

pfultz2 Nov 21, 2024

firewave Nov 21, 2024

firewave left a comment

firewave commented Nov 21, 2024

firewave left a comment •

edited

Loading

Revert valueflow split #6991

Revert valueflow split #6991

Conversation

pfultz2 commented Nov 5, 2024

pfultz2 commented Nov 8, 2024

pfultz2 commented Nov 9, 2024

firewave commented Nov 16, 2024 • edited Loading

firewave commented Nov 16, 2024

firewave commented Nov 16, 2024

danmar commented Nov 17, 2024 • edited Loading

danmar commented Nov 17, 2024 • edited Loading

firewave commented Nov 17, 2024

danmar commented Nov 18, 2024 • edited Loading

pfultz2 commented Nov 18, 2024

pfultz2 commented Nov 18, 2024

firewave commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

pfultz2 commented Nov 20, 2024

firewave commented Nov 20, 2024

firewave commented Nov 21, 2024

firewave Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

firewave Nov 21, 2024

Choose a reason for hiding this comment

pfultz2 Nov 21, 2024

Choose a reason for hiding this comment

firewave Nov 21, 2024

Choose a reason for hiding this comment

firewave left a comment

Choose a reason for hiding this comment

firewave commented Nov 21, 2024

firewave left a comment • edited Loading

Choose a reason for hiding this comment

firewave commented Nov 16, 2024 •

edited

Loading

danmar commented Nov 17, 2024 •

edited

Loading

danmar commented Nov 17, 2024 •

edited

Loading

danmar commented Nov 18, 2024 •

edited

Loading

firewave Nov 21, 2024 •

edited

Loading

firewave left a comment •

edited

Loading