◐ Shell
reader mode source ↗
Skip to content

pass TokenList as reference into ValueFlow::setValues()#4868

Merged
firewave merged 1 commit into
cppcheck-opensource:mainfrom
firewave:ptr-ref
Aug 4, 2023
Merged

pass TokenList as reference into ValueFlow::setValues()#4868
firewave merged 1 commit into
cppcheck-opensource:mainfrom
firewave:ptr-ref

Conversation

@firewave

@firewave firewave commented Mar 7, 2023

Copy link
Copy Markdown
Collaborator

This avoid lots of unchecked pointer dereferences.

There was a single case which checked it and that looked like a leftover. The only way this might have been a nullptr pointer was through several default constructors which were not used at all so I removed them.

@pfultz2

pfultz2 commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

Do we need the functionality?

If ever we want to use these classes with any generic function/algorithm(ie swap) then we do. These are fundamental operations on types.

Making classes non-regular very much limits what we can do in the future. There is already certain types of analysis we can't do because TokenList and Token are not regular classes.

@firewave

firewave commented Mar 7, 2023

Copy link
Copy Markdown
Collaborator Author

If ever we want to use these classes with any generic function/algorithm(ie swap) then we do. These are fundamental operations on types.

Ok. Can we leave the parameter a reference and just store the pointer?

Making classes non-regular very much limits what we can do in the future. There is already certain types of analysis we can't do because TokenList and Token are not regular classes.

Could you please track those issues in tickets? Thanks.

@firewave

firewave commented Mar 7, 2023

Copy link
Copy Markdown
Collaborator Author

The constParameter false negatives exposed by these changes are being tracked in https://trac.cppcheck.net/ticket/11599.

@pfultz2

pfultz2 commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

Ok. Can we leave the parameter a reference and just store the pointer?

Yes, parameters should be references, but member variables should not.

@firewave

firewave commented Mar 8, 2023

Copy link
Copy Markdown
Collaborator Author

Ok. Can we leave the parameter a reference and just store the pointer?

Yes, parameters should be references, but member variables should not.

And if I hand it out I could make it a reference again?

That sounds like you should have something like a std::shared_ptrwhich doesn't allow nullptr but that could wreck havoc with the lifetime of objects which isn't good either.

@pfultz2

pfultz2 commented Mar 9, 2023

Copy link
Copy Markdown
Contributor

We want something like gsl::non_null.

@firewave

firewave commented Mar 9, 2023

Copy link
Copy Markdown
Collaborator Author

Sometimes I am just stupid - what about #4785? I was thinking about disallowing nullptr in that.

@firewave

firewave commented Mar 9, 2023

Copy link
Copy Markdown
Collaborator Author

We already use this pattern all over the place and it is not an isolated case. So could we discuss this in the other ticket and merge this? I have a few more commits depending on this and IIRC those are just parameter and not member changes so things and even if things would just very, very slightly get worse.

@pfultz2

pfultz2 commented Mar 9, 2023

Copy link
Copy Markdown
Contributor

Sometimes I am just stupid - what about #4785?

I dont think its helpful to have a non-owning pointer propagate const. It can be simply bypassed by copying the pointer:

void f(const safe_ptr<int>& ptr) {
    // *ptr += 1 doesnt compile, but the below does
    auto ptr2 = ptr;
    *ptr += 1;
}

Its better to use const int*, which will prevent mutations to the data(and it can be used as a member variable).

@firewave

firewave commented Mar 9, 2023

Copy link
Copy Markdown
Collaborator Author

I dont think its helpful to have a non-owning pointer propagate const. It can be simply bypassed by copying the pointer:

Obviously - but those are not meant to be used outside of the class. It's to enforce the "physical constness" as far as possible and reducing the code which relies on "logical constness".

Its better to use const int*, which will prevent mutations to the data(and it can be used as a member variable).

If it is const T * to begin with there is no need for safe_ptr. This is for cases where it has to be mutable within the class but we don't want it to be mutable within const methods.

@firewave firewave force-pushed the ptr-ref branch 2 times, most recently from f12375c to 35fc752 Compare March 14, 2023 20:50
@firewave firewave force-pushed the ptr-ref branch 3 times, most recently from 91345e1 to 2b33083 Compare April 8, 2023 10:17
@firewave

firewave commented Apr 8, 2023

Copy link
Copy Markdown
Collaborator Author

Another constParameter false negative exposed by this is already being tracked as https://trac.cppcheck.net/ticket/11097.

@firewave firewave force-pushed the ptr-ref branch 2 times, most recently from e011760 to b403f49 Compare April 29, 2023 10:39

@danmar danmar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Looks good to me

@firewave firewave merged commit 77c479a into cppcheck-opensource:main Aug 4, 2023
@firewave firewave deleted the ptr-ref branch August 4, 2023 16:17
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.

3 participants