Custom linter: add many new checks and features

[LynxDev2]

• Disallow non const references (Fixes Custom linter: References should always be const #70)
• Disallow raw auto (Fixes Custom linter: Disallow raw usage of auto, only allow auto& and auto* #68)
• Disallow char16_t (Fixes Custom linter: Enforce char16_t->char16 from sead types #118)
• Mismatching functions must have a "NON_MATCHING" comment above them (Fixes Custom linter: Enforce NON_MATCHING comment above non-matching functions #71, opt-in with --check-mismatch but used automatically for the linter job no longer needs to be opt-in after optimisation)
• --all for running all checks even if some failed (Resolves Custom linter: Let custom linter run all checks even if the check before failed. #95)
• --run-clang-format for running clang-format automatically before checking each file (Resolves Custom linter: Run standard clang-format before #79)
• False fail warning when --run-clang-format isn't used (--no-warn for disabling this for the linter job)
• Remove useless --verbose option
• Require attackSensor and receiveMsg to have self and other param names
• Consistent float literals (currently .0f is enforced and .f disallowed but this might be reversed after discussion)

---

This change is 

[MonsterDruide1]

Please fix clang-format and the missing functions first.

[LynxDev2]

Done

[MonsterDruide1]

Reviewed 39 of 50 files at r1, all commit messages.
Reviewable status: 39 of 50 files reviewed, 12 unresolved discussions (waiting on @LynxDev2)

---

src/System/GameDataFile.cpp line 8 at r1 (raw file):

// NON_MATCHING

No empty line

---

lib/al/Library/Execute/ExecuteDirector.cpp line 111 at r1 (raw file):

// NON_MATCHING

(same)

---

lib/al/Library/Rail/RailRider.cpp line 8 at r1 (raw file):

// NON_MATCHING

No empty line

---

lib/al/Library/Math/MathRandomUtil.cpp line 46 at r1 (raw file):

// NON_MATCHING

No empty line

---

src/Player/PlayerInput.cpp line 0 at r1 (raw file):
Keep type of inputPort consistent (either u32 or s32)

---

lib/al/Library/Rail/Rail.cpp line 70 at r1 (raw file):

}

// (NON_MATCHING) FIXME: minor reorderings

Remove FIXME tags, having NON_MATCHING is enough
(same for all other FIXMEs below)

Suggestion:

// NON_MATCHING: minor reorderings

---

src/Npc/AchievementInfoReader.cpp line 10 at r1 (raw file):

// NON_MATCHING: minor mismatches during loop

No empty line

---

src/Player/PlayerJudgePreInputJump.cpp line 25 at r1 (raw file):

// NON_MATCHING

No empty line

---

src/Player/PlayerInputFunction.cpp line 7 at r1 (raw file):

// NON_MATCHING

No empty line

---

src/Player/PlayerPainPartsKeeper.cpp line 76 at r1 (raw file):

// NON_MATCHING

No empty line

---

lib/al/Library/Controller/InputFunction.cpp line 607 at r1 (raw file):

// NON_MATCHING

No empty line between comment and function

---

src/Player/PlayerModelHolder.cpp line 10 at r1 (raw file):

// NON_MATCHING

No empty line

[LynxDev2]

Reviewable status: 39 of 50 files reviewed, 12 unresolved discussions (waiting on @MonsterDruide1)

---

lib/al/Library/Controller/InputFunction.cpp line 607 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line between comment and function

Done.

---

lib/al/Library/Execute/ExecuteDirector.cpp line 111 at r1 (raw file):

Previously, MonsterDruide1 wrote…

(same)

Done.

---

lib/al/Library/Math/MathRandomUtil.cpp line 46 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

lib/al/Library/Rail/Rail.cpp line 70 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Remove FIXME tags, having NON_MATCHING is enough
(same for all other FIXMEs below)

Done.

---

lib/al/Library/Rail/RailRider.cpp line 8 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/Npc/AchievementInfoReader.cpp line 10 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/Player/PlayerInput.cpp line at r1 (raw file):

Previously, MonsterDruide1 wrote…

Keep type of inputPort consistent (either u32 or s32)

As discussed in DMs, there are two different functions with two different return types, so they can't be consistent

---

src/Player/PlayerInputFunction.cpp line 7 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/Player/PlayerJudgePreInputJump.cpp line 25 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/Player/PlayerModelHolder.cpp line 10 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/Player/PlayerPainPartsKeeper.cpp line 76 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

---

src/System/GameDataFile.cpp line 8 at r1 (raw file):

Previously, MonsterDruide1 wrote…

No empty line

Done.

[LynxDev2]

The NON_MATCHING comment check is still WIP so don’t review that yet

[LynxDev2]

Just optimised the NON_MATCHING check, feel free to review it

[MonsterDruide1]

Reviewed 5 of 50 files at r1, 23 of 25 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LynxDev2 and @MonsterDruide1)

---

.github/workflows/lint.yml line 27 at r3 (raw file):

      run: pip install capstone colorama cxxfilt pyelftools watchdog python-Levenshtein toml
    - name: Run custom code styling checks
      run: tools/check-format.py --no-warn

As we have a separate workflow for checking clang-format (which already fails/complains if something is wrong), we can re-format things here to produce more useful error messages.

If you want to improve it even further, add a flag --ci (continuous integration), which does not do in-place clang-format, but writes the resulting file to a new location. Then, if the file is changed, show a warning (line-numbering is messed up) before continuing to do custom format checks on the re-formatted file.

Suggestion:

run: tools/check-format.py --run-clang-format

---

tools/check-format.py line 20 at r3 (raw file):

runAllChecks = False
offsetCommentCheckFail = False
functionData = None

Avoid introducing more global variables. I see why runAllChecks is needed, but these two are certainly not:

• functionData should rather be fetched in a @cache-annotated method
• offsetCommentCheckFail is useless once the other comment(s) are done

Code quote:

offsetCommentCheckFail = False
functionData = None

---

tools/check-format.py line 21 at r3 (raw file):

offsetCommentCheckFail = False
functionData = None
foundMismatchFunctionDuplicateStart = False

Unused global variable

---

tools/check-format.py line 35 at r3 (raw file):

            right = mid - 1

    return False

I'm not too confident that the manually-mangled name from is_function_mismatchis working "good enough" to trust it here, or if we might be missing functions with mismatching state by just providing a wrong name to search here. How bad is it if we report a FAIL for every function that is not found in the CSV/function list?

---

tools/check-format.py line 335 at r3 (raw file):

    for line in c.splitlines():
        if "& " in line and line[line.find("& ") - 1] != "&" and line[line.find("& ") - 1] != " " and "CLASS&" not in line and "rotationAndTranslationFromMatrix" not in line:
            if ("const" not in line or line.find("& ") < line.find("const ")) and ("for" not in line or " : " not in line):

TODO: Check/verify these exceptions

Code quote:

        if "& " in line and line[line.find("& ") - 1] != "&" and line[line.find("& ") - 1] != " " and "CLASS&" not in line and "rotationAndTranslationFromMatrix" not in line:
            if ("const" not in line or line.find("& ") < line.find("const ")) and ("for" not in line or " : " not in line):

---

tools/check-format.py line 466 at r3 (raw file):

            FAIL("Offset comments are not allowed in headers!", line, path)
            global offsetCommentCheckFail
            offsetCommentCheckFail = True

So if one file fails, it will then block running the next check on all files?

---

tools/check-format.py line 470 at r3 (raw file):

def header_lowercase_member_offset_vars(c, path):
    for line in c.splitlines():
        if " _" in line and line.endswith(";") and "__VA_ARGS__" not in line:

Multiline-variable declarations (+ initializations) might exist.

---

tools/check-format.py line 474 at r3 (raw file):

            if offsetCommentCheckFail:
                if runAllChecks:
                    input("The offset comment check has failed which might cause a crash if the lowercase offset check is ran! (Press enter to continue regardless)")

Do not expect this to be ran interactively. It's fine to report "wrong" errors, as long as the right error is shown above this one, so ... as long as it doesn't crash the entire checker, continue here. And if it does crash the checker, you've probably done something wrong.

---

tools/check-format.py line 480 at r3 (raw file):

                if line[line.find("_") + underscoreOffset].isalpha() and line[line.find("_") + underscoreOffset].isupper():
                    FAIL("Characters in the names of offset variables need to be lowercase!", line, path)
                underscoreOffset += 1

TODO: read/understand this

Code quote:

            while line[line.find("_") + underscoreOffset] != ";" and line[line.find("_") + underscoreOffset] != " ":
                if line[line.find("_") + underscoreOffset].isalpha() and line[line.find("_") + underscoreOffset].isupper():
                    FAIL("Characters in the names of offset variables need to be lowercase!", line, path)
                underscoreOffset += 1

---

tools/check-format.py line 492 at r3 (raw file):

    lines = c.splitlines()
    for i, line in enumerate(lines):
        if "(" in line and (") {" in line or ") const {" in line) and "::" in line and "if (" not in line and "inline " not in line:

Multiline function headers are not caught with this.

---

tools/check-format.py line 508 at r3 (raw file):

            if is_function_mismatch(functionNameAndNamespaces, isConstFunction):
                if "NON_MATCHING" not in lines[i - 1] and "NON_MATCHING" not in lines[i - 2]:
                    FAIL("Functions that mismatch must have a NON_MATCHING comment above them!", line, path)

All of this is really hacky. Hardcoding namespace rs and al is not nice (stuff like alXYFunction exists). Also, you're running a lot of finds, and doing that once or twice per line to search for a line through the entire file is expensive.

I feel like the NON_MATCHING checks take more discussion and work before they can get anywhere near ready. I would suggest removing that part from this PR for now (keep the comments you added throughout files so far, but remove the relevant parts from the checker) and doing that in a separate PR, so the majority of changes/additions can be merged faster and are not blocked by this.

Code quote:

            startIndex = line.find(" ", 2, len(line) - 2) + 1
            if startIndex == -1 or startIndex > line.find("("):
                startIndex = 0
            functionNameAndNamespaces = "";
            namespaceAlLocation = c.find("namespace al {")
            if namespaceAlLocation != -1 and c.find(line) > namespaceAlLocation and "lib/al/" in path:
                functionNameAndNamespaces = "al::"
            else:
                namespaceRsLocation = c.find("namespace rs {")
                if namespaceRsLocation != -1 and c.find(line) > namespaceRsLocation and "src/" in path:
                    functionNameAndNamespaces = "rs::"
            functionNameAndNamespaces += line[startIndex:line.find("(")]
            isConstFunction = line.rfind("const") > line.rfind(")")
            if is_function_mismatch(functionNameAndNamespaces, isConstFunction):
                if "NON_MATCHING" not in lines[i - 1] and "NON_MATCHING" not in lines[i - 2]:
                    FAIL("Functions that mismatch must have a NON_MATCHING comment above them!", line, path)

---

tools/check-format.py line 587 at r3 (raw file):

    parser = argparse.ArgumentParser(
        'check-format.py', description="Verify additional formatting options next to clang-format and clang-tidy")
    parser.add_argument('-f', '--run-clang-format', action='store_true',

I'm not too happy about this shorthand, -f is --force for me... I don't have any other suggestion though.

Code quote:

'-f'

---

tools/check-format.py line 592 at r3 (raw file):

                        help="Run all checks even if one of them fails")
    parser.add_argument('--no-warn', action='store_true',
                        help="Disable format warning, only meant for automation")

Sounds a bit like you're talking about formatting on warnings

Suggestion:

help="Disable warning about running clang-format before, only meant for automation")

---

lib/al/Library/Rail/Rail.cpp line 265 at r3 (raw file):

}

// NON_MATCHING: FIXME regalloc in length calculation

Suggestion:

// NON_MATCHING: regalloc in length calculation

[LynxDev2]

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @MonsterDruide1)

---

tools/check-format.py line 474 at r3 (raw file):

Previously, MonsterDruide1 wrote…

Do not expect this to be ran interactively. It's fine to report "wrong" errors, as long as the right error is shown above this one, so ... as long as it doesn't crash the entire checker, continue here. And if it does crash the checker, you've probably done something wrong.

This is only run when -a is used and like all the other options I addded, -a is meant to be used locally only where you do have an interactive enviroment. Also I don't exactly remember what the issue was since I made this code like two months ago but I think if the other check fails, that means that the line would have something that would cause this check to freeze or crash and I wasn't able to fix it

---

tools/check-format.py line 587 at r3 (raw file):

Previously, MonsterDruide1 wrote…

I'm not too happy about this shorthand, -f is --force for me... I don't have any other suggestion though.

I could change it to -o for the second letter, but that's usually output. is ´-F´ any better?

---

tools/check-format.py line 592 at r3 (raw file):

Previously, MonsterDruide1 wrote…

Sounds a bit like you're talking about formatting on warnings

This option will be removed anyway after adding the -ci option since that's really the only place where not showing the warning is useful

[LynxDev2]

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @MonsterDruide1)

---

tools/check-format.py line 474 at r3 (raw file):

Previously, MonsterDruide1 wrote…

Do not expect this to be ran interactively. It's fine to report "wrong" errors, as long as the right error is shown above this one, so ... as long as it doesn't crash the entire checker, continue here. And if it does crash the checker, you've probably done something wrong.

This is only run when -a is used and like all the other options I addded, -a is meant to be used locally only where you do have an interactive enviroment. Also I don't exactly remember what the issue was since I made this code like two months ago but I think if the other check fails, that means that the line would have something that would cause this check to freeze or crash and I wasn't able to fix it

---

tools/check-format.py line 587 at r3 (raw file):

Previously, MonsterDruide1 wrote…

I'm not too happy about this shorthand, -f is --force for me... I don't have any other suggestion though.

I could change it to -o for the second letter, but that's usually output. is ´-F´ any better?

---

tools/check-format.py line 592 at r3 (raw file):

Previously, MonsterDruide1 wrote…

Sounds a bit like you're talking about formatting on warnings

This option will be removed anyway after adding the -ci option since that's really the only place where not showing the warning is useful

[MonsterDruide1]

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LynxDev2)

---

tools/check-format.py line 587 at r3 (raw file):

Previously, LynxDev2 wrote…

I could change it to -o for the second letter, but that's usually output. is ´-F´ any better?

Yes, that's better! Please change it accordingly.