Apply clang-tidy code improvements (#56)

This PR is a long list of changes suggested by clang-tidy, or inspired
by some of the feedback.

While this represents a lot of churn, it also allows us to enable many
clang-tidy checks, and hopefully keep a more consistent style now.

Performance, I believe is unaffected:
https://rebench.dev/SOMpp/compare/dc3c3acd1c15f1f4f9dfb41e3ed09d5b5c0a4cdf..b4f880fd6e096dfed57b1daa2b6dcf42f85824a0

Author: smarr

.clang-tidy

@@ -1,48 +1,67 @@
 ---
-Checks:          '*include*,
-                  -altera-unroll-loops,
+Checks:          '*,
                   -altera-id-dependent-backward-branch,
+                  -altera-unroll-loops,
+                  -bugprone-casting-through-void,
                   -bugprone-easily-swappable-parameters,
+                  -bugprone-reserved-identifier,
+                  -cert-dcl37-c,
+                  -cert-dcl50-cpp,
+                  -cert-dcl51-cpp,
+                  -cert-err58-cpp,
+                  -cert-msc30-c,
+                  -cert-msc32-c,
+                  -cert-msc50-cpp,
+                  -cert-msc51-cpp,
+                  -concurrency-mt-unsafe,
+                  -cppcoreguidelines-avoid-c-arrays,
+                  -cppcoreguidelines-avoid-const-or-ref-data-members,
                   -cppcoreguidelines-avoid-do-while,
+                  -cppcoreguidelines-avoid-magic-numbers,
+                  -cppcoreguidelines-avoid-non-const-global-variables,
+                  -cppcoreguidelines-macro-to-enum,
+                  -cppcoreguidelines-macro-usage,
+                  -cppcoreguidelines-no-malloc,
+                  -cppcoreguidelines-non-private-member-variables-in-classes,
+                  -cppcoreguidelines-owning-memory,
+                  -cppcoreguidelines-pro-bounds-array-to-pointer-decay,
+                  -cppcoreguidelines-pro-bounds-constant-array-index,
+                  -cppcoreguidelines-pro-bounds-pointer-arithmetic,
+                  -cppcoreguidelines-pro-type-cstyle-cast,
+                  -cppcoreguidelines-pro-type-static-cast-downcast,
+                  -cppcoreguidelines-pro-type-vararg,
+                  -cppcoreguidelines-special-member-functions,
+                  -fuchsia-default-arguments-calls,
+                  -fuchsia-default-arguments-declarations,
+                  -fuchsia-overloaded-operator,
+                  -fuchsia-statically-constructed-objects,
+                  -google-build-using-namespace,
+                  -google-default-arguments,
+                  -google-global-names-in-headers,
+                  -google-readability-casting,
+                  -hicpp-avoid-c-arrays,
+                  -hicpp-no-array-decay,
+                  -hicpp-no-malloc,
+                  -hicpp-special-member-functions,
+                  -hicpp-vararg,
                   -llvm-header-guard,
                   -llvmlibc-*,
                   -misc-no-recursion,
+                  -misc-non-private-member-variables-in-classes,
+                  -misc-use-anonymous-namespace,
+                  -modernize-avoid-bind,
+                  -modernize-avoid-c-arrays,
+                  -modernize-macro-to-enum,
                   -modernize-use-trailing-return-type,
+                  -modernize-use-using,
+                  -performance-no-int-to-ptr,
                   -readability-function-cognitive-complexity,
                   -readability-identifier-length,
+                  -readability-magic-numbers,
+                  -readability-redundant-inline-specifier,
                  '
-# '*,
-#                  '
-#                  -bugprone-exception-escape,
-#                  -concurrency-mt-unsafe,
-#                  -cppcoreguidelines-avoid-const-or-ref-data-members,
-#                  -cppcoreguidelines-avoid-magic-numbers,
-#                  -cppcoreguidelines-avoid-non-const-global-variables
-#                  -cppcoreguidelines-non-private-member-variables-in-classes,
-#                  -cppcoreguidelines-pro-bounds-constant-array-index,
-#                  -fuchsia-default-arguments-calls,
-#                  -fuchsia-default-arguments-declarations,
-#                  -fuchsia-statically-constructed-objects,
-#                  -fuchsia-virtual-inheritance,
-#                  -google-global-names-in-headers,
-#                  -hicpp-named-parameter,
-#                  -hicpp-use-auto,
-#                  -llvm-include-order,
-#                  -llvmlibc-*,
-#                  -misc-no-recursion,
-#                  -misc-non-private-member-variables-in-classes,
-#                  -misc-use-anonymous-namespace,
-#                  -readability-delete-null-pointer,
-#                  -readability-convert-member-functions-to-static,
-#                  -readability-magic-numbers,
-#                  -readability-named-parameter,
-#                  -readability-simplify-boolean-expr,
-#                  -cppcoreguidelines-special-member-functions,
-#                  -hicpp-special-member-functions,
-#                  -cppcoreguidelines-pro-bounds-pointer-arithmetic,
-#                  -cppcoreguidelines-owning-memory,
 WarningsAsErrors: '*'
-HeaderFilterRegex: '^(?!.*cppunit).*$'
+HeaderFilterRegex: '^.*/src/.*$'
 # AnalyzeTemporaryDtors: false
 FormatStyle:     none
 User:            smarr

lint.sh

@@ -4,3 +4,4 @@ clang-tidy-mp-18 --config-file=.clang-tidy src/**/*.cpp \
   --fix \
   -- -I/opt/local/include/ -fdiagnostics-absolute-paths \
   -DGC_TYPE=GENERATIONAL -DUSE_TAGGING=false -DCACHE_INTEGER=false -DUNITTESTS
+clang-format-mp-18 -i --style=file --Werror src/*.cpp  src/**/*.cpp src/**/*.h

src/Main.cpp

@@ -35,7 +35,7 @@
 #include "vmobjects/VMObject.h"
 #include "vmobjects/VMString.h"
 
-int main(int argc, char** argv) {
+int32_t main(int32_t argc, char** argv) {
     cout << "This is SOM++" << endl;
 
     if (GC_TYPE == GENERATIONAL) {

src/compiler/BytecodeGenerator.cpp

@@ -40,18 +40,18 @@
 #include "../vmobjects/VMSymbol.h"
 
 void Emit1(MethodGenerationContext& mgenc, uint8_t bytecode,
-           size_t stackEffect) {
+           int64_t stackEffect) {
     mgenc.AddBytecode(bytecode, stackEffect);
 }
 
 void Emit2(MethodGenerationContext& mgenc, uint8_t bytecode, size_t idx,
-           size_t stackEffect) {
+           int64_t stackEffect) {
     mgenc.AddBytecode(bytecode, stackEffect);
     mgenc.AddBytecodeArgument(idx);
 }
 
 void Emit3(MethodGenerationContext& mgenc, uint8_t bytecode, size_t idx,
-           size_t ctx, size_t stackEffect) {
+           size_t ctx, int64_t stackEffect) {
     mgenc.AddBytecode(bytecode, stackEffect);
     mgenc.AddBytecodeArgument(idx);
     mgenc.AddBytecodeArgument(ctx);
@@ -65,7 +65,7 @@ void EmitDUP(MethodGenerationContext& mgenc) {
     Emit1(mgenc, BC_DUP, 1);
 }
 
-void EmitPUSHLOCAL(MethodGenerationContext& mgenc, long idx, int ctx) {
+void EmitPUSHLOCAL(MethodGenerationContext& mgenc, size_t idx, size_t ctx) {
     assert(idx >= 0);
     assert(ctx >= 0);
     if (ctx == 0) {
@@ -85,7 +85,7 @@ void EmitPUSHLOCAL(MethodGenerationContext& mgenc, long idx, int ctx) {
     Emit3(mgenc, BC_PUSH_LOCAL, idx, ctx, 1);
 }
 
-void EmitPUSHARGUMENT(MethodGenerationContext& mgenc, long idx, int ctx) {
+void EmitPUSHARGUMENT(MethodGenerationContext& mgenc, size_t idx, size_t ctx) {
     assert(idx >= 0);
     assert(ctx >= 0);
 
@@ -120,7 +120,7 @@ void EmitPUSHFIELD(MethodGenerationContext& mgenc, VMSymbol* field) {
 }
 
 void EmitPUSHBLOCK(MethodGenerationContext& mgenc, VMInvokable* block) {
-    const int8_t idx = mgenc.AddLiteralIfAbsent(block);
+    const uint8_t idx = mgenc.AddLiteralIfAbsent(block);
     Emit2(mgenc, BC_PUSH_BLOCK, idx, 1);
 }
 
@@ -130,11 +130,11 @@ void EmitPUSHCONSTANT(MethodGenerationContext& mgenc, vm_oop_t cst) {
     // we also make sure that we don't miss anything in the else
     // branch of the class check
     if (CLASS_OF(cst) == load_ptr(integerClass)) {
-        if (INT_VAL(cst) == 0ll) {
+        if (INT_VAL(cst) == 0LL) {
             Emit1(mgenc, BC_PUSH_0, 1);
             return;
         }
-        if (INT_VAL(cst) == 1ll) {
+        if (INT_VAL(cst) == 1LL) {
             Emit1(mgenc, BC_PUSH_1, 1);
             return;
         }
@@ -147,7 +147,7 @@ void EmitPUSHCONSTANT(MethodGenerationContext& mgenc, vm_oop_t cst) {
         return;
     }
 
-    const int8_t idx = mgenc.AddLiteralIfAbsent(cst);
+    const uint8_t idx = mgenc.AddLiteralIfAbsent(cst);
     if (idx == 0) {
         Emit1(mgenc, BC_PUSH_CONSTANT_0, 1);
         return;
@@ -180,7 +180,7 @@ void EmitPUSHGLOBAL(MethodGenerationContext& mgenc, VMSymbol* global) {
     } else if (global == SymbolFor("false")) {
         EmitPUSHCONSTANT(mgenc, load_ptr(falseObject));
     } else {
-        const int8_t idx = mgenc.AddLiteralIfAbsent(global);
+        const uint8_t idx = mgenc.AddLiteralIfAbsent(global);
         Emit2(mgenc, BC_PUSH_GLOBAL, idx, 1);
     }
 }
@@ -191,7 +191,7 @@ void EmitPOP(MethodGenerationContext& mgenc) {
     }
 }
 
-void EmitPOPLOCAL(MethodGenerationContext& mgenc, long idx, int ctx) {
+void EmitPOPLOCAL(MethodGenerationContext& mgenc, size_t idx, size_t ctx) {
     assert(idx >= 0);
     assert(ctx >= 0);
     if (ctx == 0) {
@@ -214,7 +214,7 @@ void EmitPOPLOCAL(MethodGenerationContext& mgenc, long idx, int ctx) {
     Emit3(mgenc, BC_POP_LOCAL, idx, ctx, -1);
 }
 
-void EmitPOPARGUMENT(MethodGenerationContext& mgenc, long idx, int ctx) {
+void EmitPOPARGUMENT(MethodGenerationContext& mgenc, size_t idx, size_t ctx) {
     Emit3(mgenc, BC_POP_ARGUMENT, idx, ctx, -1);
 }
 
@@ -229,19 +229,18 @@ void EmitPOPFIELD(MethodGenerationContext& mgenc, VMSymbol* field) {
 }
 
 void EmitSEND(MethodGenerationContext& mgenc, VMSymbol* msg) {
-    const int8_t idx = mgenc.AddLiteralIfAbsent(msg);
+    const uint8_t idx = mgenc.AddLiteralIfAbsent(msg);
 
-    const int numArgs = Signature::GetNumberOfArguments(msg);
-    const size_t stackEffect = -numArgs + 1;  // +1 for the result
+    const uint8_t numArgs = Signature::GetNumberOfArguments(msg);
+    const int64_t stackEffect = -numArgs + 1;  // +1 for the result
 
     Emit2(mgenc, numArgs == 1 ? BC_SEND_1 : BC_SEND, idx, stackEffect);
 }
 
 void EmitSUPERSEND(MethodGenerationContext& mgenc, VMSymbol* msg) {
-    const int8_t idx = mgenc.AddLiteralIfAbsent(msg);
-
-    const int numArgs = Signature::GetNumberOfArguments(msg);
-    const size_t stackEffect = -numArgs + 1;  // +1 for the result
+    const uint8_t idx = mgenc.AddLiteralIfAbsent(msg);
+    const uint8_t numArgs = Signature::GetNumberOfArguments(msg);
+    const int64_t stackEffect = -numArgs + 1;  // +1 for the result
 
     Emit2(mgenc, BC_SUPER_SEND, idx, stackEffect);
 }
@@ -276,6 +275,9 @@ void EmitRETURNFIELD(MethodGenerationContext& mgenc, size_t index) {
         case 2:
             bc = BC_RETURN_FIELD_2;
             break;
+        default:
+            bc = 0;
+            break;
     }
     Emit1(mgenc, bc, 0);
 }
@@ -302,8 +304,8 @@ size_t EmitJumpOnBoolWithDummyOffset(MethodGenerationContext& mgenc,
     // This is because if the test passes, the block is inlined directly.
     // But if the test fails, we need to jump.
     // Thus, an  `#ifTrue:` needs to generated a jump_on_false.
-    uint8_t bc;
-    size_t stackEffect;
+    uint8_t bc = 0;
+    int64_t stackEffect = 0;
 
     if (needsPop) {
         bc = isIfTrue ? BC_JUMP_ON_FALSE_POP : BC_JUMP_ON_TRUE_POP;
@@ -315,36 +317,36 @@ size_t EmitJumpOnBoolWithDummyOffset(MethodGenerationContext& mgenc,
 
     Emit1(mgenc, bc, stackEffect);
 
-    size_t idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
+    size_t const idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
     mgenc.AddBytecodeArgument(0);
     return idx;
 }
 
 size_t EmitJumpWithDumyOffset(MethodGenerationContext& mgenc) {
     Emit1(mgenc, BC_JUMP, 0);
-    size_t idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
+    size_t const idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
     mgenc.AddBytecodeArgument(0);
     return idx;
 }
 
 size_t EmitJumpIfGreaterWithDummyOffset(MethodGenerationContext& mgenc) {
     Emit1(mgenc, BC_JUMP_IF_GREATER, 0);
-    size_t idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
+    size_t const idx = mgenc.AddBytecodeArgumentAndGetIndex(0);
     mgenc.AddBytecodeArgument(0);
     return idx;
 }
 
 void EmitJumpBackwardWithOffset(MethodGenerationContext& mgenc,
                                 size_t jumpOffset) {
-    uint8_t jumpBytecode =
+    uint8_t const jumpBytecode =
         jumpOffset <= 0xFF ? BC_JUMP_BACKWARD : BC_JUMP2_BACKWARD;
-    Emit3(mgenc, jumpBytecode, jumpOffset & 0xFF, jumpOffset >> 8, 0);
+    Emit3(mgenc, jumpBytecode, jumpOffset & 0xFFU, jumpOffset >> 8U, 0);
 }
 
 size_t Emit3WithDummy(MethodGenerationContext& mgenc, uint8_t bytecode,
-                      size_t stackEffect) {
+                      int64_t stackEffect) {
     mgenc.AddBytecode(bytecode, stackEffect);
-    size_t index = mgenc.AddBytecodeArgumentAndGetIndex(0);
+    size_t const index = mgenc.AddBytecodeArgumentAndGetIndex(0);
     mgenc.AddBytecodeArgument(0);
     return index;
 }

src/compiler/BytecodeGenerator.h

@@ -33,25 +33,25 @@
 #include "MethodGenerationContext.h"
 
 void Emit1(MethodGenerationContext& mgenc, uint8_t bytecode,
-           size_t stackEffect);
+           int64_t stackEffect);
 void Emit2(MethodGenerationContext& mgenc, uint8_t bytecode, size_t idx,
-           size_t stackEffect);
+           int64_t stackEffect);
 void Emit3(MethodGenerationContext& mgenc, uint8_t bytecode, size_t idx,
-           size_t ctx, size_t stackEffect);
+           size_t ctx, int64_t stackEffect);
 
 void EmitHALT(MethodGenerationContext& mgenc);
 void EmitDUP(MethodGenerationContext& mgenc);
-void EmitPUSHLOCAL(MethodGenerationContext& mgenc, long idx, int ctx);
-void EmitPUSHARGUMENT(MethodGenerationContext& mgenc, long idx, int ctx);
+void EmitPUSHLOCAL(MethodGenerationContext& mgenc, size_t idx, size_t ctx);
+void EmitPUSHARGUMENT(MethodGenerationContext& mgenc, size_t idx, size_t ctx);
 void EmitPUSHFIELD(MethodGenerationContext& mgenc, VMSymbol* field);
 void EmitPUSHBLOCK(MethodGenerationContext& mgenc, VMInvokable* block);
 void EmitPUSHCONSTANT(MethodGenerationContext& mgenc, vm_oop_t cst);
 void EmitPUSHCONSTANT(MethodGenerationContext& mgenc, uint8_t literalIndex);
 void EmitPUSHCONSTANTString(MethodGenerationContext& mgenc, VMString* str);
 void EmitPUSHGLOBAL(MethodGenerationContext& mgenc, VMSymbol* global);
 void EmitPOP(MethodGenerationContext& mgenc);
-void EmitPOPLOCAL(MethodGenerationContext& mgenc, long idx, int ctx);
-void EmitPOPARGUMENT(MethodGenerationContext& mgenc, long idx, int ctx);
+void EmitPOPLOCAL(MethodGenerationContext& mgenc, size_t idx, size_t ctx);
+void EmitPOPARGUMENT(MethodGenerationContext& mgenc, size_t idx, size_t ctx);
 void EmitPOPFIELD(MethodGenerationContext& mgenc, VMSymbol* field);
 void EmitSEND(MethodGenerationContext& mgenc, VMSymbol* msg);
 void EmitSUPERSEND(MethodGenerationContext& mgenc, VMSymbol* msg);
@@ -73,7 +73,7 @@ size_t EmitJumpIfGreaterWithDummyOffset(MethodGenerationContext& mgenc);
 void EmitJumpBackwardWithOffset(MethodGenerationContext& mgenc,
                                 size_t jumpOffset);
 size_t Emit3WithDummy(MethodGenerationContext& mgenc, uint8_t bytecode,
-                      size_t stackEffect);
+                      int64_t stackEffect);
 
 void EmitPushFieldWithIndex(MethodGenerationContext& mgenc, uint8_t fieldIdx);
 void EmitPopFieldWithIndex(MethodGenerationContext& mgenc, uint8_t fieldIdx);

src/compiler/ClassGenerationContext.cpp

@@ -42,9 +42,7 @@
 #include "../vmobjects/VMInvokable.h"
 #include "../vmobjects/VMSymbol.h"
 
-ClassGenerationContext::ClassGenerationContext()
-    : name(nullptr), superName(nullptr), classSide(false), instanceFields(),
-      instanceMethods(), classFields(), classMethods() {}
+ClassGenerationContext::ClassGenerationContext() = default;
 
 void ClassGenerationContext::AddClassField(VMSymbol* field) {
     classFields.push_back(field);
@@ -55,18 +53,18 @@ void ClassGenerationContext::AddInstanceField(VMSymbol* field) {
 }
 
 void ClassGenerationContext::SetInstanceFieldsOfSuper(VMArray* fields) {
-    size_t num = fields->GetNumberOfIndexableFields();
+    size_t const num = fields->GetNumberOfIndexableFields();
     for (size_t i = 0; i < num; i++) {
-        VMSymbol* fieldName = (VMSymbol*)fields->GetIndexableField(i);
+        auto* fieldName = (VMSymbol*)fields->GetIndexableField(i);
         assert(IsVMSymbol(fieldName));
         instanceFields.push_back(fieldName);
     }
 }
 
 void ClassGenerationContext::SetClassFieldsOfSuper(VMArray* fields) {
-    size_t num = fields->GetNumberOfIndexableFields();
+    size_t const num = fields->GetNumberOfIndexableFields();
     for (size_t i = 0; i < num; i++) {
-        VMSymbol* fieldName = (VMSymbol*)fields->GetIndexableField(i);
+        auto* fieldName = (VMSymbol*)fields->GetIndexableField(i);
         assert(IsVMSymbol(fieldName));
         classFields.push_back(fieldName);
     }
@@ -79,7 +77,7 @@ bool ClassGenerationContext::HasField(VMSymbol* field) {
     return Contains(instanceFields, field);
 }
 
-int16_t ClassGenerationContext::GetFieldIndex(VMSymbol* field) {
+int64_t ClassGenerationContext::GetFieldIndex(VMSymbol* field) {
     if (IsClassSide()) {
         return IndexOf(classFields, field);
     }
@@ -96,7 +94,7 @@ void ClassGenerationContext::AddClassMethod(VMInvokable* method) {
 
 VMClass* ClassGenerationContext::Assemble() {
     // build class class name
-    std::string ccname = string(name->GetStdString()) + " class";
+    std::string const ccname = string(name->GetStdString()) + " class";
 
     // Load the super class
     VMClass* superClass = Universe::LoadClass(superName);