pass TokenList as reference into ValueFlow::setValues()

[firewave]

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]

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

[pfultz2]

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

[firewave]

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.

[firewave]

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.

[pfultz2]

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

[firewave]

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.

[firewave]

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.

[firewave]

Or simply std::reference_wrapper?

[pfultz2]

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

[firewave]

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.

[pfultz2]

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]

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]

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

[pfultz2]

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

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

[firewave]

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]

We want something like gsl::non_null.

[firewave]

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

[firewave]

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]

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]

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]

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

[danmar]

Looks good to me