Make copy-constructible classes copyable

[pfultz2]

Lots of copy constructible classes were using const and ref members which makes the classes non-copyable due to no longer supporting a copy-assignment. I replaced the ref members with a RefThunk class which is kind of like std::reference_wrapper, but it can access the member with () so its easier to access the members. It doesnt use * or -> so it wont be confused for a possible null pointer.

Now this PR doesnt replace all reference members, just for the classes that are copy-constructible. If we want to convert a class back to use reference or const members then we can delete the copy constructor and assignment.

Furthermore, I enabled the clang-tidy check cppcoreguidelines-avoid-const-or-ref-data-members to check for these cases in the future. Here are some references explaining why this is bad practice:

• https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-constref
• https://lesleylai.info/en/const-and-reference-member-variables/
• https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/#data-members-never-const

Beyond just being bad practice, this also has prevent me from doing certain things with the ForwardAnalyzer recently that might have improved it further such as joining or swaping a forked analyzer. I intentionally made these classes copyable for this reason(and were changed to non-copyable against my feedback as well). I understand that using references help prevent dereferencing a nullptr which is why I added a RefThunk class instead of using raw pointers like previously.

[pfultz2]

A lot of the changes are just mechanical changes of . to ->. I wrote a script to filter out these changes.

filter_arrows.py

#!/usr/bin/env python3importsys, reAGGRESSIVE=any(ain ('-a', '--aggressive') forainsys.argv[1:])
ANSI=re.compile(r'\x1b\[[0-9;?]*[a-zA-Z]')
defplain(s): returnANSI.sub('', s) # strip color for detectiondefnorm(l): returnplain(l)[1:].replace('->', '.') # drop marker, normalize arrowsdefhunk_is_noise(body):
removed= [norm(l) forlinbodyifplain(l).startswith('-')]
added= [norm(l) forlinbodyifplain(l).startswith('+')]
ifnotremovedandnotadded:
returnFalsereturnsorted(removed) ==sorted(added) # every change is arrow-onlydefaggressive_body(body):
"""Cancel only the arrow-noise -/+ pairs, keep everything else."""out, rem, add= [], [], []
defflush():
nonlocalrem, addused= [False] *len(add)
anorm= [norm(a) forainadd]
forrinrem:
rn=norm(r); matched=Falseforiinrange(len(add)):
ifnotused[i] andanorm[i] ==rn: # same once arrows normalizedused[i] =matched=Truebreakifnotmatched:
out.append(r) # a real removal, keep itout.extend(afori, ainenumerate(add) ifnotused[i])
rem, add= [], []
forlineinbody:
pl=plain(line)
ifpl.startswith('-'):
ifadd: flush() # new change block beganrem.append(line)
elifpl.startswith('+'):
add.append(line)
else:
flush(); out.append(line) # context / "\ No newline"flush()
returnoutout, file_header, file_header_emitted= [], [], Falsehunk_header, hunk_body=None, []
defflush_hunk():
globalhunk_header, hunk_body, file_header_emittedifhunk_headerisNone:
returnifnothunk_is_noise(hunk_body): # drop fully-noise hunks entirelybody=aggressive_body(hunk_body) ifAGGRESSIVEelsehunk_bodyifnotfile_header_emitted:
out.extend(file_header); file_header_emitted=Trueout.append(hunk_header); out.extend(body)
hunk_header, hunk_body=None, []
forlineinsys.stdin:
pl=plain(line)
ifpl.startswith('diff --git') orpl.startswith('diff --cc'):
flush_hunk()
file_header, file_header_emitted= [line], Falseelifpl.startswith('@@'):
flush_hunk()
hunk_header, hunk_body=line, []
elifhunk_headerisnotNone:
hunk_body.append(line)
else:
file_header.append(line)
flush_hunk()
sys.stdout.write(''.join(out))

And then you can view the diff locally with: git diff $(git merge-base main HEAD) --color=always | python3 filter_arrows.py -a | less -R.

[firewave]

I would prefer if this was actually tool-driven (i.e. the clang-tidy check(s) enabled).

And if these are "non-null" pointers there is no reason to use pointers at al we should be getting a reference from the object. This makes things correct but worse as it looks like we have unchecked pointer dereferences all over the place again.

I have been working towards this for ages (with the focus on const correctness instead) in #4785 and cppcheck-opensource/simplecpp#548 but things stalled and I kept getting side tracked. Contributions on that would have been welcome.

[firewave]

I would prefer if this was actually tool-driven (i.e. the clang-tidy check(s) enabled).

Sorry, I somehow I overlooked this because I am feeling more under the weather than usual and should not be reviewing things.

[pfultz2]

And if these are "non-null" pointers there is no reason to use pointers at al we should be getting a reference from the object.

These only construct from the reference, so they cant be constructed as a null pointer. This is where it behaves like std::reference_wrapper and not gsl::non_null.

A reference cant be used as a member variable because they cant rebind(making the class non-copyable) where as NonNullPtr can rebind. The reason I named it as Ptr is because you need to dereference it like a pointer and it rebinds like a pointer.

[pfultz2]

I have been working towards this for ages (with the focus on const correctness instead) in #4785 and cppcheck-opensource/simplecpp#548 but things stalled and I kept getting side tracked.

I looked into making NonNullPtr propagate the const, but this would require a much larger change as some parameters need to remove const, and some of the loggers are being accessed non-const from const methods which requires a mutable variable(I dont know of an easy way to drop the mutable here).

However, there is one caveat with this. The const is still fairly shallow as you can just copy the variable and then modify it(this isnt a problem for ValuePtr because it copies a new value and not a reference). This is why propagate_const is non-copyable, but we cannot make NonNullPtr non-copyable as its whole purpose is to allow classes to be copyable.

[firewave]

I looked into making NonNullPtr propagate the const, but this would require a much larger change as some parameters need to remove const, and some of the loggers are being accessed non-const from const methods which requires a mutable variable(I dont know of an easy way to drop the mutable here).

I think this is mostly caused by ErrorLogger functions which should be const even if writing to the stream can be considered "technically not const".

Also we need to split the actual output from the ErrorLogger as we have code which just wants to output and has nothing to do with errors (like debug logging and dumping stuff). I have this prepared but as this is quite intrusive I haven't gotten around to it.

However, there is one caveat with this. The const is still fairly shallow as you can just copy the variable and then modify it(this isnt a problem for ValuePtr because it copies a new value and not a reference). This is why propagate_const is non-copyable, but we cannot make NonNullPtr non-copyable as its whole purpose is to allow classes to be copyable.

If you want to, you can get obvious get around it but it greatly improves things and helps the tooling to suggest more constness (see the simplecpp check).

These only construct from the reference, so they cant be constructed as a null pointer. This is where it behaves like std::reference_wrapper and not gsl::non_null.

A reference cant be used as a member variable because they cant rebind(making the class non-copyable) where as NonNullPtr can rebind. The reason I named it as Ptr is because you need to dereference it like a pointer and it rebinds like a pointer.

I meant we should not be providing a pointer from the class i.e. using . instead of ->. I do not like it at all that code is being to handling pointers instead of references - that always implies that it could be null although it is impossible.

[pfultz2]

However, there is one caveat with this. The const is still fairly shallow as you can just copy the variable and then modify it(this isnt a problem for ValuePtr because it copies a new value and not a reference). This is why propagate_const is non-copyable, but we cannot make NonNullPtr non-copyable as its whole purpose is to allow classes to be copyable.

If you want to, you can get obvious get around it but it greatly improves things and helps the tooling to suggest more constness (see the simplecpp check).

I agree, just something to be aware of.

These only construct from the reference, so they cant be constructed as a null pointer. This is where it behaves like std::reference_wrapper and not gsl::non_null.
A reference cant be used as a member variable because they cant rebind(making the class non-copyable) where as NonNullPtr can rebind. The reason I named it as Ptr is because you need to dereference it like a pointer and it rebinds like a pointer.

I meant we should not be providing a pointer from the class i.e. using . instead of ->. I do not like it at all that code is being to handling pointers instead of references - that always implies that it could be null although it is impossible.

That is an unfortunate due to not allowing . operator to be overloaded. However, this tradeoff is really a minor annoyance compared to not having copyable(or movable) types.

Best practices(like from CppCoreGuideline) also consider this an acceptable tradeoff as well.

But there are some alternatives that I can think of. We could use the () operator instead of -> so it becomes m().a instead of m->a, just one extra character. I would probably rename the class to NonNullRef instead since it wont work like a pointer in this case. We could also do .get() method but that seems more verbose. What do you think?

[danmar]

I replaced this suppression #8714 . If you revert this change and rebase it should work.

[pfultz2]

I meant we should not be providing a pointer from the class i.e. using . instead of ->. I do not like it at all that code is being to handling pointers instead of references - that always implies that it could be null although it is impossible.

That is an unfortunate due to not allowing . operator to be overloaded. However, this tradeoff is really a minor annoyance compared to not having copyable(or movable) types.

Best practices(like from CppCoreGuideline) also consider this an acceptable tradeoff as well.

But there are some alternatives that I can think of. We could use the () operator instead of -> so it becomes m().a instead of m->a, just one extra character. I would probably rename the class to NonNullRef instead since it wont work like a pointer in this case. We could also do .get() method but that seems more verbose. What do you think?

@firewave@danmar Before I do any work on this, would making this use () instead of * or -> be an acceptable change? That would make it look like a getter rather than a pointer so it wont be confused with possibly a null pointer dereference. I am thinking of renaming the class to Ref or something else, Any suggestions?

[pfultz2]

I switched it to (), but this leads to some false positives because we dont resolve the function or types across the ():

structA {
[[noreturn]]voidg(int);
};
template<classT>
structThunk {
T& operator()() const;
};
voidf(Thunk<A> thunk, int* p) {
if (!p)
thunk().g(0);
*p = 1; // <- false positive here
}

#8755 fixes this issue and needs to be merged in first.

[danmar]

Lots of copy constructible classes were using const and ref members which makes the classes non-copyable due to no longer supporting a copy-assignment.

I don't feel that is bad if we never need copy-assignment.

If copy-assignment is needed then yes we need to solve const/ref members.

I like to use const and references.

[danmar]

why is this () needed now? the old code looks preferable to me.

[pfultz2]

@firewave raised a concern that -> looks like it might possibly deref a null pointer even though its never null. Looking at the std library, this is pretty much the case for all library classes that use -> as well including std::optional(ie std::nullopt) and std::polymorphic(ie valueless_after_move). So it is a reasonable concern.

Therefore, I changed it to use () instead of * or ->. I have no preference either way.

[pfultz2]

why is this () needed now? the old code looks preferable to me.

Ah wait, I think I misunderstood this comment. You mean why is () instead of just using the . directly. That is because I wrap the class in a RefThunk(this all explained in the description of the PR).

Originally I made classes use a pointer directly but then they were changed to use a ref to avoid null pointers. Now I changed it to use RefThunk which is a pointer internally(so the class can be assignable) but it does not support a null state(its not default constructible or constructible from a pointer). So now we get the best of both worlds which is a copy-assignment and no null state.

Also, in the future, the class could be made to propagate the const further to improve const correctness(something the vanilla refs do no support). I didnt do it in this PR as there requires a much larger change as mentioned here.

[pfultz2]

Lots of copy constructible classes were using const and ref members which makes the classes non-copyable due to no longer supporting a copy-assignment.

I don't feel that is bad if we never need copy-assignment.

If copy-assignment is needed then yes we need to solve const/ref members.

We need copy assignment if we want to do joins for forked analyzers(something I was trying to experiment with recently and it failed because of missing copy-assignment) as already mentioned in the PR description. Furthermore, it prevents using the class with std library functions and algorithms like swap, sort, partition, etc.

Its a fundamental architectural decision that may not be needed at first, but when it is needed, it would require a
significant refactor(that may not be feasible) that we should just design the classes correctly from the start(which I did originally but it was changed against my feedback). Thus I would like to use clang tidy to enforce this practice(which is also considered best practice for c++).

I like to use const and references.

And you can, but if you want to use it for a class member, you need to either use the RefThunk class or delete the copy constructor. There could be some classes in this PR that could have a deleted copy constructor but for now I didnt make that change and just focused on keeping the same behavior.