Skip to content

Conversation

@idshibanov
Copy link
Collaborator

Speeding up the CodeChecker action.

@idshibanov idshibanov added improvement New feature, request or improvement GitHub Actions GitHub Actions CI labels Nov 3, 2025
@idshibanov
Copy link
Collaborator Author

I think because of the --ctu-all flag "Pre-analysis" step still executes for the full codebase, not sure if required or not.

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Nov 3, 2025

Hi @idshibanov the main reason why I switched from scan-build to Codechecker at the time was better support for CTU analysis. If you skip some files, then its quality will decrease and the use of CTU analysis may make no sense at all. For instance, in the example from the link above, if foo() in foo.cpp returned 1 at first, and then its implementation was changed to return zero, we will not be able to find the error if we do not analyze BOTH main.cpp and foo.cpp.

@idshibanov
Copy link
Collaborator Author

Hi @idshibanov the main reason why I switched from scan-build to Codechecker at the time was better support for CTU analysis. If you skip some files, then its quality will decrease and the use of CTU analysis may make no sense at all. For instance, in the example from the link above, if foo() in foo.cpp returned 1 at first, and then its implementation was changed to return zero, we will not be able to find the error if we do not analyze BOTH main.cpp and foo.cpp.

Got it. The current state of PR leaves CTU analysis as is but speeds up total processing time (30min->6min). Check the action run: https://github.com/ihhub/fheroes2/actions/runs/19033889740/job/54353445915?pr=10338. If you're good with this, let's merge.

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Nov 6, 2025

Got it. The current state of PR leaves CTU analysis as is but speeds up total processing time (30min->6min).

There is one thing that bothers me a bit.

First, a little background. With current settings in master, CTU analysis is done in two phases. The first phase is to "precompile" cpp files as if they were headers. This is the relatively fast phase that saves a bit of time later during the second phase. During the second phase, if file b.cpp uses the functions from file a.cpp, then the "precompiled" version of a.cpp is "merged" into b.cpp (something like what the linker with LTO enabled does) and the actual analysis is performed.

Now is the thing that bothers me. Consider that this PR is merged and a.cpp is changed in a way that harmless in itself but breaks the b.cpp. Since b.cpp is not changed, it will get into the skip file and will not be processed during the second phase, only a.cpp will be processed in itself (without checking how exactly the b.cpp calls the code from a.cpp), so all the CTU machinery will be useless. What do you think about this scenario?

@idshibanov
Copy link
Collaborator Author

idshibanov commented Nov 7, 2025

I guess to solve your concerns we need to find a middle ground and probably implement a script that builds an impact set that pulls in headers. If larger than a threshold only run a full scan.

@oleg-derevenetz
Copy link
Collaborator

I guess to solve your concerns we need to find a middle ground and probably implement a script that builds an impact set that pulls in headers. If larger than a threshold only run a full scan.

In general, I'm not against disabling the CTU (by removing the corresponding --ctu-XXX flags) and proceeding with the skip file. Perhaps CTU is not worth the resources it consumes :)

@ihhub ihhub self-requested a review November 9, 2025 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GitHub Actions GitHub Actions CI improvement New feature, request or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants