fix #12771: Progress value is not increased #6993

ludviggunne · 2024-11-11T09:26:17Z

No description provided.

lib/importproject.cpp

firewave · 2024-11-12T13:51:26Z

I have similar code which deals with the language. I guess the basic logic can be re-used/combined from the changes.

firewave · 2024-11-13T10:36:40Z

This also needs tests for the cases the progress was not correctly reported.

ludviggunne · 2024-11-14T13:18:51Z

I have similar code which deals with the language. I guess the basic logic can be re-used/combined from the changes.

Hm, I'm not sure what you mean, do you have a PR for it?

firewave · 2024-11-16T15:37:00Z

Hm, I'm not sure what you mean, do you have a PR for it?

Not yet - #6978 is in preparation of the change I referred to. But I am not too sure about it - the context-less usages (i.e. no file, no language) which will only lead to issue with specific usage are giving me headaches. Maybe I use your use case to proceed with this.

cli/processexecutor.cpp

test/cli/proj2_test.py

firewave · 2024-11-20T12:46:29Z

cli/threadexecutor.cpp

@@ -106,7 +108,7 @@ class ThreadData
        if (mItNextFileSettings != mFileSettings.end()) {
            file = nullptr;
            fs = &(*mItNextFileSettings);
-            fileSize = 0;


To make sure we always fetch the filesize we probably should initialize/set it to -1 everywhere and valid that later on. Can be done at a later date though.

lib/importproject.cpp

firewave · 2024-11-20T12:49:58Z

FYI when we need to test something related to inputs there is three different cases to consider:

files specified via CLI
files specified via a .cppcheck project (basically emulates the CLI options but sometimes has its own logic)
any other project (for ease that would be a compilation database)

ludviggunne · 2024-11-23T16:49:34Z

FYI when we need to test something related to inputs there is three different cases to consider:

Okay, thanks! I'll be focusing on other tickets for a while so I'll pick this up at a later time.

firewave · 2024-11-25T14:02:50Z

I'll be focusing on other tickets for a while so I'll pick this up at a later time.

Don't end up like me... 🙄

firewave · 2024-12-20T21:23:12Z

I would prefer to have the code which gets the filesize in path.cpp.

lib/filesettings.h

ludviggunne · 2024-12-22T17:22:50Z

I noticed the percentage reported in SingleExecutor for FileSettings is based on the number of files instead of the file sizes, is this intentional?

cppcheck/cli/singleexecutor.cpp

Line 67 in 85eadd8

reportStatus(c, mFileSettings.size(), c, mFileSettings.size());

firewave · 2024-12-22T17:42:02Z

I noticed the percentage reported in SingleExecutor for FileSettings is based on the number of files instead of the file sizes, is this intentional?

No idea - I would need to look at the history.

But I wonder why we calculate it based on on the file size anyways. That makes no sense because we are using the size of the actual input file. A file with just a few lines itself might end up with much more code which is being analyzed. So that would only make sense if we used the size of preprocessed code.

I think we should just consider the amount of files.

ludviggunne · 2024-12-22T18:04:38Z

I think we should just consider the amount of files.

That makes sense. I'm not sure how the workflow would look for that, should it be it's own ticket?

danmar · 2024-12-27T09:51:30Z

But I wonder why we calculate it based on on the file size anyways. That makes no sense because we are using the size of the actual input file. A file with just a few lines itself might end up with much more code which is being analyzed. So that would only make sense if we used the size of preprocessed code.

Originally it was number of files..

Then at some point we were discussing some project in the chat. I was running Cppcheck on the project. I had checked a number of the files. So the report said "X% done". And in the chat I said something like "I have checked X% already but well there are big files remaining so probably it's going to take longer time."
Then somebody in the chat suggested that we would base "X% done" on the file sizes.

It's still a very rough estimate. As you pointed out it depends a lot on what headers are included. And it can depend a lot on how many branches etc there are. A huge file with trivial code can run quickly.

I don't have super strong opinion.. but generally.. based on number of files the guess will be off more imho. A small file would generally go faster to analyse than a large file.

danmar · 2024-12-27T10:06:20Z

A small file would generally go faster to analyse than a large file.

That is my "best guess".. I can't prove it. :-)

ludviggunne · 2024-12-27T11:22:57Z

I don't have super strong opinion.. but generally.. based on number of files the guess will be off more imho. A small file would generally go faster to analyse than a large file.

Would it be possible to base it on the length of the token list?
Anyway, I suppose we can stick to filesizes in this PR?

danmar · 2024-12-27T13:09:46Z

Would it be possible to base it on the length of the token list?

nah.. I believe it would be more accurate but that would increase the analysis time. we would need to open all the files and count all the tokens to know the total number of tokens.

firewave · 2024-12-27T13:54:51Z

The value is arbitrary and inaccurate either way - so I would go with the simpler implementation i.e. file count only. It would also match the default ('-j1') behavior.

ludviggunne · 2024-12-29T15:18:08Z

I guess we actually have both right now, with 1/3 files checked 42%.

firewave · 2024-12-30T16:40:37Z

I guess we actually have both right now, with 1/3 files checked 42%.

Fair enough.

test/cli/proj2_test.py

Co-authored-by: Oliver Stöneberg <[email protected]>

danmar · 2025-01-09T11:30:04Z

cli/cmdlineparser.cpp

@@ -245,6 +245,9 @@ bool CmdLineParser::fillSettingsFromArgs(int argc, const char* const argv[])
            if (mSettings.library.markupFile(fs.filename())) {
                fs.file.setLang(Standards::Language::C);
            }
+            std::string error = fs.updateFileSize();
+            if (!error.empty())
+                mLogger.printError(error);


if you use mLogger.printError then it feels like we should abort. But I think it's unfortunate to abort if there are missing files. Could we write some proper cppcheck warnings during the analysis instead if a file cannot be opened.

How about just setting filesize to -1 or something:

when total file size is calculated ignore files that have size -1

during analysis ensure there is some proper cppcheck message when a file can not be opened.

I suppose the files can just be removed from the list (after showing a warning)? That way we don't have to care about it when calculating the total file size.

If that error happens we should bail out - otherwise we would silently (nobody cares about the warnings because they are not visible in a headless system - yet) drop a file from the analysis which might cause issues to slip through or lead to inconsistent results between runs.

This should only happen if there are filesystem issues or somebody is modifying the data. You should avoid that. e.g. in the CLion IDE integration I pass a fixed temporary file to the analysis to avoid that a file which might constantly being changed is being read.

If mLogger.printError(error); is used and Cppcheck just aborts then I see the risk that Cppcheck plugins will silently just fail and the user get no proper information. For instance the vscode plugin does not show the Cppcheck printError output it just does not recognize its output and aborts. It just says "Failed to execute Cppcheck" or something like that.

nobody cares about the warnings because they are not visible in a headless system - yet

I don't know what you talk about. It would be pointless to run Cppcheck at all if nobody cares about the warnings. I want that we report proper Cppcheck warning output so the plugins have a good chance to show the output.

This should only happen if there are filesystem issues or somebody is modifying the data.

Yeah sure.. but we do run into this sometimes. And it would be nice to be able to scan a project even if some files are autogenerated during build or whatever..

firewave · 2025-01-09T12:25:07Z

I guess we actually have both right now, with 1/3 files checked 42%.

Fair enough.

I still think we should drop the file size percentage. Looking at how much time and discussion has already been put into this and it is something a high percentage of the users will probably never see as they use headless systems (i.e. CI/IDE integrations). It seems the time might be better spent on something else than making an arbitrary value arbitrary in a different way...

danmar · 2025-01-10T19:28:34Z

I still think we should drop the file size percentage

I don't want to drop it.

ludviggunne changed the title ~~fix #12771~~ fix #12771: Progress value is not increased Nov 11, 2024

danmar reviewed Nov 11, 2024

View reviewed changes

lib/importproject.cpp Outdated Show resolved Hide resolved

ludviggunne force-pushed the 12771 branch from de9c5a7 to bba87f1 Compare November 11, 2024 13:05

ludviggunne force-pushed the 12771 branch from 8d68396 to 81eef61 Compare November 18, 2024 11:48

danmar reviewed Nov 19, 2024

View reviewed changes

cli/processexecutor.cpp Outdated Show resolved Hide resolved

firewave reviewed Nov 20, 2024

View reviewed changes

test/cli/proj2_test.py Outdated Show resolved Hide resolved

firewave reviewed Nov 20, 2024

View reviewed changes

lib/importproject.cpp Outdated Show resolved Hide resolved

ludviggunne force-pushed the 12771 branch from fd4aea1 to 9871aa7 Compare December 22, 2024 15:54

firewave reviewed Dec 22, 2024

View reviewed changes

lib/filesettings.h Show resolved Hide resolved

ludviggunne force-pushed the 12771 branch from a962b46 to f9f3382 Compare December 22, 2024 16:13

firewave reviewed Dec 30, 2024

View reviewed changes

test/cli/proj2_test.py Outdated Show resolved Hide resolved

ludviggunne and others added 23 commits January 3, 2025 13:16

use _stati64 on 64 bit windows

53d7585

move (most of the) file size logic to FileWithDetails

b25503d

use accumulate

09936ca

update dmake dependencies

4571588

add const

8129131

remove unused includes

8f5df68

add test

8cd166d

move test

3933832

rebase

037c6ad

fix path

037cffe

calculate percentage in test

3b7ed50

account for different file orders

11f7685

account for spaces in filenames

5a97406

move filesize logic to path.cpp

758b7ed

set file sizes in CmdLineParser::fillSettingsFromArgs

4dc6684

store filesize in pipeFile map

ccad3f0

remove -j2 in progress test

456de96

update dmake

2e09d44

don't use ssize_t

46e2a80

fix typo

006c9f4

base percentage on file size in SingleExecutor

cd4ff8a

add test for .cppcheck files

a9570ae

Update test/cli/proj2_test.py

feb624b

Co-authored-by: Oliver Stöneberg <[email protected]>

ludviggunne force-pushed the 12771 branch from 75f0812 to feb624b Compare January 3, 2025 12:32

ludviggunne added 2 commits January 3, 2025 13:43

fix FileWithDetails constructor

c3a9e9c

add test for files specified on command line

2ddba93

ludviggunne marked this pull request as ready for review January 8, 2025 16:38

danmar reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #12771: Progress value is not increased #6993

fix #12771: Progress value is not increased #6993

ludviggunne commented Nov 11, 2024

firewave commented Nov 12, 2024

firewave commented Nov 13, 2024

ludviggunne commented Nov 14, 2024

firewave commented Nov 16, 2024

firewave Nov 20, 2024

firewave commented Nov 20, 2024 •

edited

Loading

ludviggunne commented Nov 23, 2024

firewave commented Nov 25, 2024

firewave commented Dec 20, 2024

ludviggunne commented Dec 22, 2024 •

edited

Loading

firewave commented Dec 22, 2024

ludviggunne commented Dec 22, 2024

danmar commented Dec 27, 2024 •

edited

Loading

danmar commented Dec 27, 2024

ludviggunne commented Dec 27, 2024

danmar commented Dec 27, 2024

firewave commented Dec 27, 2024

ludviggunne commented Dec 29, 2024

firewave commented Dec 30, 2024

danmar Jan 9, 2025

ludviggunne Jan 9, 2025

firewave Jan 9, 2025

danmar Jan 10, 2025

firewave commented Jan 9, 2025

danmar commented Jan 10, 2025

fix #12771: Progress value is not increased #6993

Are you sure you want to change the base?

fix #12771: Progress value is not increased #6993

Conversation

ludviggunne commented Nov 11, 2024

firewave commented Nov 12, 2024

firewave commented Nov 13, 2024

ludviggunne commented Nov 14, 2024

firewave commented Nov 16, 2024

firewave Nov 20, 2024

Choose a reason for hiding this comment

firewave commented Nov 20, 2024 • edited Loading

ludviggunne commented Nov 23, 2024

firewave commented Nov 25, 2024

firewave commented Dec 20, 2024

ludviggunne commented Dec 22, 2024 • edited Loading

firewave commented Dec 22, 2024

ludviggunne commented Dec 22, 2024

danmar commented Dec 27, 2024 • edited Loading

danmar commented Dec 27, 2024

ludviggunne commented Dec 27, 2024

danmar commented Dec 27, 2024

firewave commented Dec 27, 2024

ludviggunne commented Dec 29, 2024

firewave commented Dec 30, 2024

danmar Jan 9, 2025

Choose a reason for hiding this comment

ludviggunne Jan 9, 2025

Choose a reason for hiding this comment

firewave Jan 9, 2025

Choose a reason for hiding this comment

danmar Jan 10, 2025

Choose a reason for hiding this comment

firewave commented Jan 9, 2025

danmar commented Jan 10, 2025

firewave commented Nov 20, 2024 •

edited

Loading

ludviggunne commented Dec 22, 2024 •

edited

Loading

danmar commented Dec 27, 2024 •

edited

Loading