Skip to content

Fix #13894 (Update simplecpp to 1.4.2) #7554

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danmar
Copy link
Owner

@danmar danmar commented May 30, 2025

No description provided.

@danmar
Copy link
Owner Author

danmar commented May 30, 2025

@Tal500 the CI fails because:

   - /tmp/pytest-of-runner/pytest-0/popen-gw0/test_unused_function_include0/test.h:4:0: style: The function 'f' is never used. [unusedFunction]
  + ../../../../../tmp/pytest-of-runner/pytest-0/popen-gw0/test_unused_function_include0/test.h:4:0: style: The function 'f' is never used. [unusedFunction]

could you comment on this. would it make sense to use the absolute path here ? I don't have a very strong opinion but at least the absolute path is shorter.

@Tal500
Copy link

Tal500 commented May 30, 2025

@Tal500 the CI fails because:

   - /tmp/pytest-of-runner/pytest-0/popen-gw0/test_unused_function_include0/test.h:4:0: style: The function 'f' is never used. [unusedFunction]
  + ../../../../../tmp/pytest-of-runner/pytest-0/popen-gw0/test_unused_function_include0/test.h:4:0: style: The function 'f' is never used. [unusedFunction]

could you comment on this. would it make sense to use the absolute path here ? I don't have a very strong opinion but at least the absolute path is shorter.

This is a tricky question. The idea is that relativiness is determined only by the inlcude paths and the include dirs(-I), and since I didn't want to make things more complicated, I didn't want it to be dependant on whether the cpp files that are given in the simplecpp/cppcheck commands are with absolute paths or not.
I didn't want that in the case that the current directory and the source files directories are matching, there will be absolute paths always if the input source cpp file is given in an absolute path.

You can write your comments here, but I will be N/A for a few days now

@danmar
Copy link
Owner Author

danmar commented May 30, 2025

The idea is that relativiness is determined only by the inlcude paths and the include dirs(-I)

hmm spontanously this sounds like how I want it to work. If I provide the -I/tmp/.. on the command line I prefer that the path is absolute. and if I provide -I../.... on the command line I prefer the relative path in the warnings.

@danmar
Copy link
Owner Author

danmar commented Jun 2, 2025

@Tal500 in this case there is no include dir specified:

daniel@laptop:~/cppchecksolutions/cppcheck$ ./cppcheck --enable=all /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp 
Checking /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp ...
../../../../tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/test.h:5:6: error: Division by zero. [zerodiv]
x=100/0;
     ^
nofile:0:0: information: Active checkers: 167/856 (use --checkers-report=<filename> to see details) [checkersReport]

daniel@laptop:~/cppchecksolutions/cppcheck$ cat /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp

#include "test.h"
        
daniel@laptop:~/cppchecksolutions/cppcheck$ cat /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/test.h 

#ifndef TEST_H
#define TEST_H

x=100/0;

#endif

and imho the absolute path would be preferable in this case.

@Tal500
Copy link

Tal500 commented Jun 3, 2025

@Tal500 in this case there is no include dir specified:

daniel@laptop:~/cppchecksolutions/cppcheck$ ./cppcheck --enable=all /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp 
Checking /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp ...
../../../../tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/test.h:5:6: error: Division by zero. [zerodiv]
x=100/0;
     ^
nofile:0:0: information: Active checkers: 167/856 (use --checkers-report=<filename> to see details) [checkersReport]

daniel@laptop:~/cppchecksolutions/cppcheck$ cat /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/1.cpp

#include "test.h"
        
daniel@laptop:~/cppchecksolutions/cppcheck$ cat /tmp/pytest-of-daniel/pytest-7/test_addon_namingng0/test.h 

#ifndef TEST_H
#define TEST_H

x=100/0;

#endif

and imho the absolute path would be preferable in this case.

So stating this clearly, the choice algorithm for determination of relative or absolute flavor (always w.r.t. the pwd) will be as such - A path (both for (1) top-level source files and (2) header files - i.e. files that are introduced by #include) will be absolute if and only if at least one of the condition is true:

  1. The top-level source file (for headers it is the one that includes it) is given as an input file to simplecpp in its absolute form, except for the case that this is a header file that its inclusion was resolved by inclusion dirs (i.e. -I) - and this condition is false in that latter case (note that sometimes condition 3 can be true in that case, resulting in absolute flavor).
  2. For header files only that are not system includes(i.e. #include "..."): The include path is absolute.
  3. For header files that were resolved by inclusion dirs (i.e. -I): The resolved inclusion dir is absolute.

Whenever the same file is introduced twice or more, only the first occurrence that simplecpp sees will determine the absoluteness flavor.

Note: excluding 1, this is the current behaviour.

WDUT? Shall we implement it this way? Testing the new condition 1 is supposed to be easy now in our integration testing suite.

@danmar
Copy link
Owner Author

danmar commented Jun 9, 2025

WDUT? Shall we implement it this way? Testing the new condition 1 is supposed to be easy now in our integration testing suite.

@Tal500 sorry for late reply.

I think let's try this.

@Tal500
Copy link

Tal500 commented Jun 15, 2025

WDUT? Shall we implement it this way? Testing the new condition 1 is supposed to be easy now in our integration testing suite.

@Tal500 sorry for late reply.

I think let's try this.

implemented in danmar/simplecpp#444

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