Skip to content

ValueFlow: extracted more code into separate files - #6449

Merged
firewave merged 10 commits into
cppcheck-opensource:mainfrom
firewave:vf-inc-22
Jul 20, 2024
Merged

ValueFlow: extracted more code into separate files#6449
firewave merged 10 commits into
cppcheck-opensource:mainfrom
firewave:vf-inc-22

Conversation

@firewave

Copy link
Copy Markdown
Collaborator

No description provided.

@firewave
firewaveforce-pushed the vf-inc-22 branch 2 times, most recently from 8ed72f1 to 0863c01CompareMay 26, 2024 21:49
@firewave
firewaveforce-pushed the vf-inc-22 branch 3 times, most recently from abb5fdc to 48074b5CompareJune 3, 2024 11:41
@firewave

Copy link
Copy Markdown
CollaboratorAuthor

@danmar can this be merged? It is not different in principal from the previous change I did in this and it blocks a lot of follow-ups (including prototyping some of the bigger refactoring still needed).

@danmar

Copy link
Copy Markdown
Collaborator

Isn't this going too extreme? Putting every function in a separate file. There are many small files that are less than 100 lines of code.

@firewave

firewave commented Jun 17, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Isn't this going too extreme? Putting every function in a separate file. There are many small files that are less than 100 lines of code.

This is just because these are the low-hanging cases. And it is quite possible we might enhance the steps in the future so there might be more code.

This also allows us to actually write a unit test for each of these. It also makes it way easier to review the code (and I have a bunch of smaller cleanups coming).

Comment threadlib/vf_bitand.cpp
@danmar

Copy link
Copy Markdown
Collaborator

I would still say it's pretty extreme to put each function in a separate file. We could end up with 1000's of files if you continue to refactor cppcheck.

As far as I see we can still write the same unit tests if we have several functions in a file.

@firewave

Copy link
Copy Markdown
CollaboratorAuthor

I would still say it's pretty extreme to put each function in a separate file. We could end up with 1000's of files if you continue to refactor cppcheck.

We are not extracting an infinite number of functions but a finite number of passes with differing complexity.

Some other functionality will also be factored out but those involve a lot of code.

@firewave
firewaveforce-pushed the vf-inc-22 branch 2 times, most recently from d70f2b5 to b9b851fCompareJuly 1, 2024 17:18

@danmardanmar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we split it up into larger chunks. splitting out each function into a separate file that is extreme to me. how about a file for all "non-analyzer" passes.

I would not want that we continue and split up astutils tokenizer etc like this.

@firewave

Copy link
Copy Markdown
CollaboratorAuthor

splitting out each function into a separate file that is extreme to me. how about a file for all "non-analyzer" passes.

this was always the plan from the beginning so the previous PR should have never been approved and there was some back and froth while this could have been brought up.

I would not want that we continue and split up astutils tokenizer etc like this.

I am not going to do that. valueflow.cpp is extremely monolithic, is slow to compile and is just way too big. That partially applies to some of the other files as well but there is not much which could be factored out. the ValueFlow is different in that way.

@firewave
firewave merged commit 25d7921 into cppcheck-opensource:mainJul 20, 2024
@firewave
firewave deleted the vf-inc-22 branch July 20, 2024 11:19
pfultz2 added a commit to pfultz2/cppcheck that referenced this pull request Nov 5, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@firewave@danmar