Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ValueFlow: extracted more code into separate files #6449

Merged
merged 10 commits into from
Jul 20, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave firewave force-pushed the vf-inc-22 branch 2 times, most recently from 8ed72f1 to 0863c01 Compare May 26, 2024 21:49
@firewave firewave force-pushed the vf-inc-22 branch 3 times, most recently from abb5fdc to 48074b5 Compare June 3, 2024 11:41
@firewave
Copy link
Collaborator Author

@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
Owner

danmar commented Jun 17, 2024

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
Copy link
Collaborator Author

firewave commented Jun 17, 2024

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).

@danmar
Copy link
Owner

danmar commented Jun 26, 2024

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
Collaborator Author

firewave commented Jul 1, 2024

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 firewave force-pushed the vf-inc-22 branch 2 times, most recently from d70f2b5 to b9b851f Compare July 1, 2024 17:18
Copy link
Owner

@danmar danmar left a comment

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
Collaborator Author

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 danmar:main Jul 20, 2024
63 checks passed
@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 free to 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