pass `TokenList` as reference into `ValueFlow::setValues()` by firewave · Pull Request #4868 · cppcheck-opensource/cppcheck · GitHub

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.

I would prefer to use pointers as members instead of references. Otherwise it makes the class non regular(not assignable or default constructible).

Also, when using the object after move, a pointer can be a nullptr which is easier to diagnose the issue.

Do we need the functionality? They are local objects that are just being passed around.

The point is to avoid that it could ever be a nullptr. Since they are just used internally and the object will always exist it is impossible to happen but I think it would be better not to allow it anyways.

Also, when using the object after move, a pointer can be a nullptr which is easier to diagnose the issue.

That is a good point. But until #4845 is merged they are not being moved at all. As it seems to complicate things and probably won't give any meaningful improvement I am open to dropping it anyways.

There is cppcoreguidelines-avoid-const-or-ref-data-members check in clang tidy which will find these issues. We should enable this check.

We are also using way too many struct without any access modifiers allowing modification of any members of them if objects are not longer const. We have countless hidden issues because of that which luckily rarely cause issues (as they are mostly in the tests) but just yesterday I ran into one in production code which might cause files to be analyzed with wrong settings.

Okay - pointer to const objects are still allowed. That makes things just slightly better. But still being able to re-assign them in the class is dangerous. So to properly get a working const * const-like construct you would need some data class with accessors.

class C1
{
public:
    bool f() const;
};

class C2Data
{
protected:
    C2Data(const C1 &c1) : mC1(&c1) {}

    const C1 &c1() const { return *mC1; }
private:
    const C1 * mC1;
};

class C2 : public C2Data
{
public:
    C2(const C1 &c1) : C2Data(c1) {}
    bool f() {
        return c1().f();
    }
};

static void f()
{
    const C1 c1;
    C2 c2(c1);
}

Maybe you have to adjust the inheritance access but if I ever knew something about that I have forgotten it.

Still somebody would be able to do stupid things within C2Data. That would require some other check that tells you the data class is only there for storage and accessors.

Or simply std::reference_wrapper?

Yea you could use std::reference_wrapper here(which is similar to gsl::non_null)

Alright. I will start testing it after I am done with most of my backlog. That will probably spill into the next dev cycle since I keep unearthing issues addressing other things and I don't want to pile yet another refactoring on top of it.

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.

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.

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

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

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

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.

We want something like gsl::non_null.

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

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.

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).

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.

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

Looks good to me

pfultz2 reviewed Mar 7, 2023

View reviewed changes

firewave force-pushed the ptr-ref branch 2 times, most recently from f12375c to 35fc752 Compare March 14, 2023 20:50

danmar reviewed Mar 27, 2023

View reviewed changes

Comment thread lib/valueflow.cpp

firewave force-pushed the ptr-ref branch 3 times, most recently from 91345e1 to 2b33083 Compare April 8, 2023 10:17

firewave force-pushed the ptr-ref branch from 2b33083 to 2c3420e Compare April 8, 2023 11:20

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

danmar reviewed Apr 30, 2023

View reviewed changes

firewave force-pushed the ptr-ref branch from b403f49 to 57afc83 Compare August 2, 2023 09:50

pass TokenList as reference into ValueFlow::setValues()

f0d6a1e

firewave force-pushed the ptr-ref branch from 57afc83 to f0d6a1e Compare August 2, 2023 09:54

firewave merged commit 77c479a into cppcheck-opensource:main Aug 4, 2023

firewave deleted the ptr-ref branch August 4, 2023 16:17

pfultz2 mentioned this pull request Jul 7, 2026

Make copy-constructible classes copyable #8705

Open

               struct ValueFlowAnalyzer : Analyzer {
-                  const TokenList* tokenlist;
+                  const TokenList& tokenlist;

pass `TokenList` as reference into `ValueFlow::setValues()` by firewave · Pull Request #4868 · cppcheck-opensource/cppcheck · GitHub

Uh oh!

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

pass TokenList as reference into ValueFlow::setValues()#4868firewave merged 1 commit intocppcheck-opensource:maincppcheck-opensource/cppcheck:mainfrom firewave:ptr-reffirewave/cppcheck:ptr-refCopy head branch name to clipboard

Conversation

firewave commented Mar 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfultz2 commented Mar 7, 2023

Uh oh!

firewave commented Mar 7, 2023

Uh oh!

firewave commented Mar 7, 2023

Uh oh!

pfultz2 commented Mar 8, 2023

Uh oh!

firewave commented Mar 8, 2023

Uh oh!

pfultz2 commented Mar 9, 2023

Uh oh!

firewave commented Mar 9, 2023

Uh oh!

firewave commented Mar 9, 2023

Uh oh!

pfultz2 commented Mar 9, 2023

Uh oh!

firewave commented Mar 9, 2023

Uh oh!

Uh oh!

firewave commented Apr 8, 2023

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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