ValueFlow: start splitting it into multiple files #4748

firewave · 2023-01-28T11:11:59Z

valueflow.cpp is very monolithic which makes it hard to manage. This is splitting several parts into separate files as discussed in #4642.

firewave · 2023-01-28T11:16:22Z

@pfultz2 Please have a look.
This is the initial approach which works quite well. I already did further functions up to the first more complex case valueFlowForwardLifetime() were it gets quite granular but we can discuss this after decided if this approach is fine.

I will also get rid of the using namespace ValueFlow within the split files since it makes things harder to understand when the increased granularity kicks in.

firewave · 2023-01-29T13:36:50Z

I am reworking this a bit and will fix the matchcompiler and CMake.

firewave · 2023-01-31T13:26:24Z

Still needs matchcompiler support.

Also need to make sure it doesn't impact the performance negatively now the code is no longer completely visible to some of the callers.

pfultz2 · 2023-02-04T21:35:37Z

lib/vf/number.cpp

+
+#include "common.h"
+
+void ValueFlow::number(TokenList *tokenlist) {


I dont think we should shorten the names like that as this could conflict with other functions in the future. I understand why we wouldn't want ValueFlow::valueFlowNumber, but we should have a clear naming scheme so they wont conflict.

The other transformations have a naming scheme like simplifySomething. I am thinking something like analyzeNumber or assignNumber?

That sounds good. Will adjust.

pfultz2 · 2023-02-04T21:36:51Z

lib/vf/enumvalue.cpp

+#include "token.h"
+#include "valueflow.h"
+
+void ValueFlow::enumValue(SymbolDatabase * symboldatabase, const Settings * settings)


I think this should be inside namespace ValueFlow{ ... }. This will simplify calling the other functions.

I had using namespace ValueFlow; in the source files which is better since it doesn't increase the indentation. But I went back on it. I guess I add it back.

pfultz2 · 2023-02-04T21:46:16Z

lib/vf/number.h

+
+namespace ValueFlow
+{
+    void number(TokenList *tokenlist);


This is only used be valueflow.cpp. I think it would be better to put all the declarations in valueflow.cpp or in a single header that valueflow.cpp includes.

Everything in the vf folder is only used by valueflow.cpp. But having a convenience header with all of them in it would be fine I guess.

pfultz2 · 2023-02-04T21:49:03Z

lib/vf/common.h

+
+    std::string debugString(const ValueFlow::Value& v);
+
+    void setSourceLocation(ValueFlow::Value& v,


I dont think setSourceLocation or debugString need to be put in the header as its only used by setTokenValue.

They will be used in more than one of the split off source file. Otherwise I could have directly moved those in. There's probably room for more cleanups/refactoring after it is all done.

pfultz2 · 2023-02-04T21:51:02Z

lib/vf/common.h

+                       SourceLocation loc = SourceLocation::current());
+}
+
+#endif // vfCommonH


I think it would be better to name the header settokenvalue.h. Using a common header can bring in a lot of unnecessary dependencies.

I plan to split up common later on. It helps with the incremental approach of the changes.

Since we only split internals of valueflow.cpp into separate files which are only included by valueflow.cpp there will be no additional dependencies compared to before. It is correct though that they might be unneeded in scope of the newly created smaller parts though. That should not cause any issues though and can be cleaned up afterwards.

Already done in this PR since it involves a lot of code.

pfultz2 · 2023-02-04T21:52:07Z

I will also get rid of the using namespace ValueFlow within the split files since it makes things harder to understand when the increased granularity kicks in.

Wrapping in namespace ValueFlow { ... } will get rid of that.

firewave · 2023-02-04T22:45:59Z

I will also get rid of the using namespace ValueFlow within the split files since it makes things harder to understand when the increased granularity kicks in.

Wrapping in namespace ValueFlow { ... } will get rid of that.

I don't like that the indentation is being increased by that. I think it's nicer to simply add a single line and having them all aligned to the first line.

firewave · 2023-02-04T22:47:35Z

This currently doesn't play well with the matchcompiler. I have it working in CMake but the regular make is still an issue. It also seems there's some code in it which we don't need anymore.

pfultz2 · 2023-02-10T17:12:17Z

I don't like that the indentation is being increased by that. I think it's nicer to simply add a single line and having them all aligned to the first line.

We should update the formatting to not indent for namespace scopes.

firewave · 2024-04-20T17:21:14Z

I will give this another stab after #6298 has been merged.

firewave · 2024-04-20T17:56:02Z

Since it complicates things and does not match the structure the project has been using all along I will omit introducing a subfolder and introduce an underscore in the filenames instead (although that also doesn't match what has been done so far).

firewave · 2024-04-21T13:15:51Z

Re-did the first commit, will re-add the others later.

Having this in individual files might also allow us to write unit (and maybe performance tests) for the respective analyze steps.

firewave · 2024-04-22T10:40:26Z

We could already merge this as a start or I could move yet another step. IMO this change is way too big to do it in a single commit.

After all the steps have been moved out I will do the further suggested split of the common code.

#4748 exposed a different order of files using the Windows code. This was caused by it only sorting the files in case of recursive parsing. But we need to sort all the files and not just the directory contents as the directory order might also be different dependent on the filesystem. This also allows the code to be a bit closer between the implementations.

firewave · 2024-05-06T12:04:47Z

Any feedback on this? I would like to proceed on this so it is done within this dev cycle. Also so we could maybe do some work on #6097.

…oduced `vf_analyze.h` to group all analyze headers

… separate files

danmar

lgtm.

firewave force-pushed the vf-inc branch from 331fbc1 to 9264885 Compare January 30, 2023 15:40

pfultz2 reviewed Feb 4, 2023

View reviewed changes

firewave force-pushed the vf-inc branch 3 times, most recently from 3b494c2 to 4f10788 Compare February 6, 2023 11:03

firewave mentioned this pull request Sep 1, 2023

avoid duplicated workflow invocations for pushes in PRs made from root repository branches #5377

Merged

firewave mentioned this pull request Nov 16, 2023

moved some classes/structs into anonymous namespace #5669

Merged

firewave mentioned this pull request Mar 9, 2024

support CMAKE_UNITY_BUILD #6104

Closed

firewave mentioned this pull request Apr 19, 2024

Add --check-level=fast option that reuses much faster valueflow analysis from 1.90 #6097

Closed

firewave force-pushed the vf-inc branch 2 times, most recently from b336045 to 5bba4f5 Compare April 21, 2024 13:14

firewave force-pushed the vf-inc branch from 5bba4f5 to 6e6f0a4 Compare April 21, 2024 13:30

firewave mentioned this pull request Apr 22, 2024

FileLister: ensure sorting of files / some cleanups #6322

Merged

firewave force-pushed the vf-inc branch from 6e6f0a4 to 1956d91 Compare April 22, 2024 09:49

firewave changed the title ~~ValueFlow: split into multiple files~~ ValueFlow: start splitting it into multiple files Apr 22, 2024

firewave force-pushed the vf-inc branch 6 times, most recently from f7748a0 to 88bff95 Compare April 26, 2024 15:40

firewave marked this pull request as ready for review April 26, 2024 15:46

firewave added 2 commits May 25, 2024 10:01

ValueFlow: extracted valueFlowEnumValue() into separate file / intr…

7432a14

…oduced `vf_analyze.h` to group all analyze headers

ValueFlow: extracted valueFlowNumber() and related common code into…

d4f935d

… separate files

firewave force-pushed the vf-inc branch from 88bff95 to d4f935d Compare May 25, 2024 08:01

danmar approved these changes May 26, 2024

View reviewed changes

firewave merged commit 98d56b8 into danmar:main May 26, 2024

firewave deleted the vf-inc branch May 26, 2024 19:15


		#include "common.h"

		void ValueFlow::number(TokenList *tokenlist) {


		std::string debugString(const ValueFlow::Value& v);

		void setSourceLocation(ValueFlow::Value& v,

ValueFlow: start splitting it into multiple files #4748

ValueFlow: start splitting it into multiple files #4748

Uh oh!

Conversation

firewave commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave commented Jan 28, 2023

Uh oh!

firewave commented Jan 29, 2023

Uh oh!

firewave commented Jan 31, 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!

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!

pfultz2 commented Feb 4, 2023

Uh oh!

firewave commented Feb 4, 2023

Uh oh!

firewave commented Feb 4, 2023

Uh oh!

pfultz2 commented Feb 10, 2023

Uh oh!

firewave commented Apr 20, 2024

Uh oh!

firewave commented Apr 20, 2024

Uh oh!

firewave commented Apr 21, 2024

Uh oh!

firewave commented Apr 22, 2024

Uh oh!

firewave commented May 6, 2024

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

firewave commented Jan 28, 2023 •

edited

Loading