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

#!/usr/bin/env python3
import sys, re

AGGRESSIVE = any(a in ('-a', '--aggressive') for a in sys.argv[1:])

ANSI = re.compile(r'\x1b\[[0-9;?]*[a-zA-Z]')
def plain(s): return ANSI.sub('', s)                   # strip color for detection
def norm(l):  return plain(l)[1:].replace('->', '.')   # drop marker, normalize arrows

def hunk_is_noise(body):
    removed = [norm(l) for l in body if plain(l).startswith('-')]
    added   = [norm(l) for l in body if plain(l).startswith('+')]
    if not removed and not added:
        return False
    return sorted(removed) == sorted(added)            # every change is arrow-only

def aggressive_body(body):
    """Cancel only the arrow-noise -/+ pairs, keep everything else."""
    out, rem, add = [], [], []
    def flush():
        nonlocal rem, add
        used = [False] * len(add)
        anorm = [norm(a) for a in add]
        for r in rem:
            rn = norm(r); matched = False
            for i in range(len(add)):
                if not used[i] and anorm[i] == rn:      # same once arrows normalized
                    used[i] = matched = True
                    break
            if not matched:
                out.append(r)                           # a real removal, keep it
        out.extend(a for i, a in enumerate(add) if not used[i])
        rem, add = [], []
    for line in body:
        pl = plain(line)
        if pl.startswith('-'):
            if add: flush()                             # new change block began
            rem.append(line)
        elif pl.startswith('+'):
            add.append(line)
        else:
            flush(); out.append(line)                   # context / "\ No newline"
    flush()
    return out

out, file_header, file_header_emitted = [], [], False
hunk_header, hunk_body = None, []

def flush_hunk():
    global hunk_header, hunk_body, file_header_emitted
    if hunk_header is None:
        return
    if not hunk_is_noise(hunk_body):                    # drop fully-noise hunks entirely
        body = aggressive_body(hunk_body) if AGGRESSIVE else hunk_body
        if not file_header_emitted:
            out.extend(file_header); file_header_emitted = True
        out.append(hunk_header); out.extend(body)
    hunk_header, hunk_body = None, []

for line in sys.stdin:
    pl = plain(line)
    if pl.startswith('diff --git') or pl.startswith('diff --cc'):
        flush_hunk()
        file_header, file_header_emitted = [line], False
    elif pl.startswith('@@'):
        flush_hunk()
        hunk_header, hunk_body = line, []
    elif hunk_header is not None:
        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.

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.

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.

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.

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.

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.

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?

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?

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

struct A {
    [[noreturn]] void g(int);
};

template<class T>
struct Thunk {
    T& operator()() const;
};

void f(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.

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.

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.