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

Add Original Name to types for Typedefs #5968

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

wienans
Copy link
Contributor

@wienans wienans commented Feb 9, 2024

Trying to Fix the issue described here: https://sourceforge.net/p/cppcheck/discussion/general/thread/5775ff1666/

For the small example purpose script it works. Actually it shows the valueType-originalTypeName. But i am not 100% sure, what is done. I printed the tok2 and tok3 and in my simple case both where the same. And if i add either of both the originalName it also works. But for now i couldn't deduct what exactly tok2 and tok3 are doing

@wienans wienans marked this pull request as draft February 9, 2024 15:55
@wienans wienans marked this pull request as ready for review February 9, 2024 15:56
@wienans wienans marked this pull request as draft February 9, 2024 15:56
@wienans wienans marked this pull request as ready for review February 9, 2024 19:41
@wienans wienans changed the title WIP: Add Original Name to types for Typedefs Add Original Name to types for Typedefs Feb 10, 2024
@chrchr-github
Copy link
Collaborator

Please add a test for this in testsimplifytypedef.cpp.
You can use Token::findsimplematch() to get the right token.

@wienans
Copy link
Contributor Author

wienans commented Feb 10, 2024

Okay will try it likely tomorrow.
didn’t find any instructions on how to build and run the tests in Linux cmake in the readme. Is there any description or hints?

@chrchr-github
Copy link
Collaborator

I'm not on Linux, but don't you get a testrunner executable?

@pfultz2
Copy link
Contributor

pfultz2 commented Feb 11, 2024

You can run the tests with make check.

@wienans
Copy link
Contributor Author

wienans commented Feb 11, 2024

Hi @chrchr-github
didn't find any testrunner executable in the cmake build project on linux but setup visual studio on windows an build worked, as well as testrunner.exe

It will now test that the original name of the type is set.

I wanted to additionally test that the valueType-originalTypeName for the dump file variable was also set.

But the access via the token->valueType() wasn't possible, as it does not have a method to get the original Name and i got an access error about "C++ The type of a pointer to an incomplete class () is not allowed." Not exactly sure if the test of original Name in the Value Type is even necessary or if the copying of originalName of the type to the variable is tested also somewhere else.

@wienans
Copy link
Contributor Author

wienans commented Feb 11, 2024

Hi @pfultz2 thanks for the note. as mentioned above i tried windows and it worked for me too

@wienans
Copy link
Contributor Author

wienans commented Feb 12, 2024

Added also Tests that the valueTypeOriginalName is present in the Variable, actually the thing which gets printed into the dump file and in what i am interested. I also tested the main branch and both tests fail on the main branch therefore double-checked that this wasn't tested before in any way. If all Actions pass i would appreciate a merge :)

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.

looks good to me.

@danmar
Copy link
Owner

danmar commented Feb 12, 2024

didn't find any testrunner executable in the cmake build project on linux

you need to pass "-DBUILD_TESTS=ON" to cmake then you should get the testrunner.

Personally I don't usually use cmake nor windows.. and usually build and run the tests like so: cd cppcheck && make all -j12 && ./testrunner.

@wienans
Copy link
Contributor Author

wienans commented Feb 12, 2024

didn't find any testrunner executable in the cmake build project on linux

you need to pass "-DBUILD_TESTS=ON" to cmake then you should get the testrunner.

Personally I don't usually use cmake nor windows.. and usually build and run the tests like so: cd cppcheck && make all -j12 && ./testrunner.

Found the cmake solution. And added PR to document in the readme as you already saw in #5973. not sure why make caused build problems for me maybe some dependency issue on my side, but cmake worked for me and that’s why I didn’t dig deeper.

thanks for approval. Hope the contribution will be helpful for others too after merge and the 2.14 gets released.

@danmar
Copy link
Owner

danmar commented Feb 12, 2024

could you test what happens if there is code:

typedef unsigned char * u8ptr;
u8ptr x;

I would assume that the "char" token will not get a originalName? Should insertTokens set the originalName instead?

@wienans
Copy link
Contributor Author

wienans commented Feb 12, 2024

could you test what happens if there is code:

typedef unsigned char * u8ptr;
u8ptr x;

I would assume that the "char" token will not get a originalName? Should insertTokens set the originalName instead?

By experience from the function pointer where all the information is written to the * it might be the same with your pointer typedef.

In the first place I wondered why the typedef resolves into such a strange pattern of tokens. Actually the tokens directly before a functionpointer variable are (*. And the info is written on the *

I will test it tomorrow, with your pointer type.

@wienans
Copy link
Contributor Author

wienans commented Feb 13, 2024

Hi @danmar

for your Pointer Typedef i get this token result in the dump file, as expected the originalName is written to the *

    <token id="28669edbe90" file="src/main.c" linenr="60" column="5" str="char" scope="28669ec17d0" type="name" isUnsigned="true"/>
    <token id="28669ee0770" file="src/main.c" linenr="60" column="5" str="*" scope="28669ec17d0" type="op" isArithmeticalOp="true" originalName="uint8_ptr"/>
    <token id="28669edc430" file="src/main.c" linenr="60" column="15" str="pubTest" scope="28669ec17d0" type="name" varId="17" exprId="17" variable="28669e8bb90" valueType-type="char" valueType-sign="unsigned" valueType-pointer="1" valueType-reference="None" valueType-originalTypeName="uint8_ptr"/>

Similar to the function pointer

    <token id="1f709aecb80" file="src/main.c" linenr="50" column="17" str="void" scope="1f7099dea80" type="name"/>
    <token id="1f709af1ff0" file="src/main.c" linenr="50" column="17" str="(" scope="1f7099dea80" link="1f709af19f0" valueType-type="void" valueType-reference="None"/>
    <token id="1f709af1c90" file="src/main.c" linenr="50" column="17" str="*" scope="1f7099dea80" type="op" isArithmeticalOp="true" originalName="eventCallback_t"/>
    <token id="1f709aee070" file="src/main.c" linenr="50" column="33" str="pvFunc" scope="1f7099dea80" type="name" varId="12" exprId="12" variable="1f709aa0550" valueType-type="void" valueType-pointer="1" valueType-reference="None" valueType-originalTypeName="eventCallback_t"/>

It handles the token directly before it as the type.

@danmar
Copy link
Owner

danmar commented Feb 13, 2024

@wienans I let you decide. You are the one who wants to use this info..

Is it ok that originalName is set on the "*" token only or should we try to set it on all/other tokens? maybe all the name tokens. maybe on all the leading name tokens..

@wienans
Copy link
Contributor Author

wienans commented Feb 13, 2024

For my purpose using it together with the namingng addon it’s the convenient if it is directly in the token before the variable which is what it is doing now. Otherwise I would need to search for the real type and in cases of char const * const* and other crazy combinations it’s best to have it easily accessible within the first token before the variable name. As we talking about namingng addon I will contribute the non specific part which uses this feature in the future. So that the namingng addon works with more complex type structures.

conclusion if the PR is fine from your side we can merge it.

@danmar danmar merged commit 2a27412 into danmar:main Feb 14, 2024
64 checks passed
@wienans wienans deleted the fix/originalTypeName branch February 27, 2024 15:11
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.

4 participants