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 NonNullPtr class which is kind of a mix of gsl::non_null and std::reference_wrapper. This helps prevent assigning it as null since it only accepts a reference and not a 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 NonNullPtr 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 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.

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