Skip to content

Fix relative paths, again #418

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 14 commits into
base: master
Choose a base branch
from
Open

Conversation

Tal500
Copy link
Contributor

@Tal500 Tal500 commented Feb 16, 2025

Undo #415 , and hopefully finally fix the Windows string conversion error.
The windows string conversion part is based on https://gist.github.com/RyanJeong/04a4b92879ab10edfd9656d0aa4f3b77

Fixes #402

@Tal500
Copy link
Contributor Author

Tal500 commented Feb 25, 2025

Note: See the compilation test for both UNICODE and non-UNICODE mode at compiler explorer (my best guess)

@danmar
Copy link
Owner

danmar commented Mar 18, 2025

ok I try it out here: danmar/cppcheck#7381

@danmar
Copy link
Owner

danmar commented Mar 18, 2025

Well this does not work neither:

https://github.com/danmar/cppcheck/actions/runs/13923783226/job/38962964585?pr=7381

CI output:

       "D:\a\cppcheck\cppcheck\cppcheck.sln" (default target) (1) ->
       "D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj" (default target) (4) ->
       (ClCompile target) -> 
         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(152,13): error C2375: 'startsWith': redefinition; different linkage [D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj]
         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(2726,8): error C2264: 'startsWith': error in function definition or declaration; function not called [D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj]

@danmar
Copy link
Owner

danmar commented Mar 18, 2025

@Tal500 I am sorry for slow reply.. hope you can fix it anyway.

@Tal500
Copy link
Contributor Author

Tal500 commented Mar 20, 2025

@Tal500 I am sorry for slow reply.. hope you can fix it anyway.

Well this does not work neither:

https://github.com/danmar/cppcheck/actions/runs/13923783226/job/38962964585?pr=7381

CI output:

       "D:\a\cppcheck\cppcheck\cppcheck.sln" (default target) (1) ->
       "D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj" (default target) (4) ->
       (ClCompile target) -> 
         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(152,13): error C2375: 'startsWith': redefinition; different linkage [D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj]
         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(2726,8): error C2264: 'startsWith': error in function definition or declaration; function not called [D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj]

There must be something wrong with how that you run this test suite. It comlains:

         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(152,13): error C2375: 'startsWith': redefinition; different linkage [D:\a\cppcheck\cppcheck\lib\cppcheck.vcxproj]
         D:\a\cppcheck\cppcheck\externals\simplecpp\simplecpp.cpp(2726,8): error C2264: 'startsWith': error in function definition or declaration; function not called 

But the thing that I just fixed in this PR is to have only a single, "static", function definition of startsWith() in the file "simplecpp.cpp", as you can see by yourself.
My previous PR that didn't work had this issue of double definition, you must be running the previous PR in the checks somehow.

@Tal500
Copy link
Contributor Author

Tal500 commented Mar 20, 2025

@Tal500 I am sorry for slow reply.. hope you can fix it anyway.

On second glance, the problem seems to be very stupid. Microsoft MSVC complains about redefinition of a function even when one of them is defined to be with "static" visibility, when you do static linkage.
The solution is to stupidely add a postfix "_" to this function name of startsWith() to make all the compiling system happy (will do it now).

EDIT: namespacing seems like a more sane solution

EDIT2: hhhhh seems you did exactly what I said already in the code:

simplecpp/simplecpp.cpp

Lines 70 to 80 in 78f0f7c

// TODO: added an undercore since this conflicts with a function of the same name in utils.h from Cppcheck source when building Cppcheck with MSBuild
static bool isStringLiteral_(const std::string &s)
{
return s.size() > 1 && (s[0]=='\"') && (*s.rbegin()=='\"');
}
// TODO: added an undercore since this conflicts with a function of the same name in utils.h from Cppcheck source when building Cppcheck with MSBuild
static bool isCharLiteral_(const std::string &s)
{
// char literal patterns can include 'a', '\t', '\000', '\xff', 'abcd', and maybe ''
// This only checks for the surrounding '' but doesn't parse the content.

So I'm joining the party for now. I suggest using namespace simplecpp_utils {}; using namespace simplecpp_utils workaround for the future.

Please try delivering these changes again.

@danmar
Copy link
Owner

danmar commented Apr 5, 2025

@Tal500 I tried this quickly. There is a testcase in cppcheck that fails:

https://github.com/danmar/cppcheck/actions/runs/14283167360/job/40035087732?pr=7381

The full CI message:


D:\a\cppcheck\cppcheck\test\testpreprocessor.cpp:2513(TestPreprocessor::testMissingIncludeCheckConfig): Assertion failed. 
Expected: 
test.c:1:0: information: Include file: \"missing.h\" not found. [missingInclude]\n
test.c:2:0: information: Include file: <header.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:3:0: information: Include file: <missing2.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:6:0: information: Include file: \"header4.h\" not found. [missingInclude]\n
test.c:9:0: information: Include file: \"D:\a\cppcheck\cppcheck/missing3.h\" not found. [missingInclude]\n
test.c:11:0: information: Include file: <D:\a\cppcheck\cppcheck/missing4.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

Actual: 
test.c:1:0: information: Include file: \"missing.h\" not found. [missingInclude]\n
test.c:2:0: information: Include file: <header.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:3:0: information: Include file: <missing2.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:5:0: information: Include file: <header3.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:6:0: information: Include file: \"header4.h\" not found. [missingInclude]\n
test.c:9:0: information: Include file: \"D:\a\cppcheck\cppcheck/missing3.h\" not found. [missingInclude]\n
test.c:11:0: information: Include file: <D:\a\cppcheck\cppcheck/missing4.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

So it seems there is a unwanted message:

test.c:5:0: information: Include file: <header3.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

@danmar
Copy link
Owner

danmar commented Apr 5, 2025

The selfchecks in the CI also fail but I am not sure if that is caused by these changes right now.

@danmar
Copy link
Owner

danmar commented Apr 5, 2025

The test TestPreprocessor::testMissingIncludeCheckConfig sounds more like it should be done in simplecpp spontanously. Maybe the recently added pytest testing can test this.

@Tal500
Copy link
Contributor Author

Tal500 commented Apr 5, 2025

@Tal500 I tried this quickly. There is a testcase in cppcheck that fails:

https://github.com/danmar/cppcheck/actions/runs/14283167360/job/40035087732?pr=7381

The full CI message:


D:\a\cppcheck\cppcheck\test\testpreprocessor.cpp:2513(TestPreprocessor::testMissingIncludeCheckConfig): Assertion failed. 
Expected: 
test.c:1:0: information: Include file: \"missing.h\" not found. [missingInclude]\n
test.c:2:0: information: Include file: <header.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:3:0: information: Include file: <missing2.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:6:0: information: Include file: \"header4.h\" not found. [missingInclude]\n
test.c:9:0: information: Include file: \"D:\a\cppcheck\cppcheck/missing3.h\" not found. [missingInclude]\n
test.c:11:0: information: Include file: <D:\a\cppcheck\cppcheck/missing4.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

Actual: 
test.c:1:0: information: Include file: \"missing.h\" not found. [missingInclude]\n
test.c:2:0: information: Include file: <header.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:3:0: information: Include file: <missing2.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:5:0: information: Include file: <header3.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n
test.c:6:0: information: Include file: \"header4.h\" not found. [missingInclude]\n
test.c:9:0: information: Include file: \"D:\a\cppcheck\cppcheck/missing3.h\" not found. [missingInclude]\n
test.c:11:0: information: Include file: <D:\a\cppcheck\cppcheck/missing4.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

So it seems there is a unwanted message:

test.c:5:0: information: Include file: <header3.h> not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n

I claim that the "valid" value there is WRONG and the "actual" string value is CORRECT.
The missing header3 should be reported twice, independently, since the full path inclusion in "..." isn't the same as the inclusion of the missing header file by a "system" include within <...>, UNLESS the header is found and is resolved to the same path.

@Tal500
Copy link
Contributor Author

Tal500 commented Apr 5, 2025

The test TestPreprocessor::testMissingIncludeCheckConfig sounds more like it should be done in simplecpp spontanously. Maybe the recently added pytest testing can test this.

It's a matter of taste. I do not mind to have a double coverage, one that checks within the library and one that checks the library within the usage.

@danmar
Copy link
Owner

danmar commented Apr 6, 2025

It's a matter of taste. I do not mind to have a double coverage, one that checks within the library and one that checks the library within the usage.

yeah I am not against that we have tests in cppcheck but it seems to me that we could test this directly in simplecpp also..

@Tal500
Copy link
Contributor Author

Tal500 commented Apr 6, 2025

It's a matter of taste. I do not mind to have a double coverage, one that checks within the library and one that checks the library within the usage.

yeah I am not against that we have tests in cppcheck but it seems to me that we could test this directly in simplecpp also..

The test cases there seems to be redundant, you can then omit them at all.
If more test cases are needed in your opinion, we can work this out, but I prefer them not to be a blocker for this push, because the coverage is very high (and I claim that it checks effectively all the cases appearing in the cppcheck tests)

@danmar
Copy link
Owner

danmar commented Apr 7, 2025

I claim that the "valid" value there is WRONG and the "actual" string value is CORRECT. The missing header3 should be reported twice, independently, since the full path inclusion in "..." isn't the same as the inclusion of the missing header file by a "system" include within <...>, UNLESS the header is found and is resolved to the same path.

I am not sure am I missing something?

There is a file system/header3.h. We provide the include path system. And the code is:

#include <header3.h>

So we should find that shouldn't we?

@danmar
Copy link
Owner

danmar commented Apr 7, 2025

This manual test is the same isn't it?

daniel@laptop:~/cppchecksolutions/cppcheck$ mkdir system
daniel@laptop:~/cppchecksolutions/cppcheck$ touch system/header3.h
daniel@laptop:~/cppchecksolutions/cppcheck$ echo "#include <header3.h>" > test.c
daniel@laptop:~/cppchecksolutions/cppcheck$ gcc -E -Isystem test.c

gcc does not complain about a missing header3.h file.

@Tal500
Copy link
Contributor Author

Tal500 commented Apr 7, 2025

This manual test is the same isn't it?

daniel@laptop:~/cppchecksolutions/cppcheck$ mkdir system
daniel@laptop:~/cppchecksolutions/cppcheck$ touch system/header3.h
daniel@laptop:~/cppchecksolutions/cppcheck$ echo "#include <header3.h>" > test.c
daniel@laptop:~/cppchecksolutions/cppcheck$ gcc -E -Isystem test.c

gcc does not complain about a missing header3.h file.

I didn't notice that the file is actually being created, so the behavior you claimed is correct.
However, if you do the following next command you'd see that simplecpp detect this case correctly:

> ./simplecpp -Isystem test.c
(no output)

After debugging, I uploaded a fix now. The error was that the wchar to char conversion of the current directory was failing for some reason in windows 2019 release (a very weird bug, really don't know the true cause).
I found an easier implementation for current directory extraction in Windows in this codebase (path.cpp), so I stole this code and modified the implementation of currentDirectoryOSCalc():

static std::string currentDirectoryOSCalc() {
    const std::size_t size = 4096;
    char currentPath[size];

#ifndef _WIN32
    if (getcwd(currentPath, size) != nullptr)
#else
    if (_getcwd(currentPath, size) != nullptr)
#endif
        return std::string(currentPath);

    return "";
}

For debugging and verifying, I tried to create a pull request in my fork of cppcheck, to reproduce your checks. The checks looks very good now - Tal500/cppcheck#1

Sadly, I was unable to reproduce this weird behavior in simplecpp automated CI workflow, so while the logical test code is already covered by the python tests, the CI windows setup that reproduced it is not.

Please try to integrate it again.

@danmar
Copy link
Owner

danmar commented Apr 7, 2025

Thanks I will try it out again

@danmar
Copy link
Owner

danmar commented Apr 7, 2025

CI is running now:
danmar/cppcheck#7381

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.

absolute and relative header paths aren't resolved to be the same header
2 participants