Comply with two clang-tidy bugprone checks, and add clang-tidy comments. (#267)

Author: garrisonhess

.clang-tidy

@@ -26,14 +26,12 @@ Checks:     '
             performance-*,
             portability-*,
             readability-*,
-            -bugprone-branch-clone,
             -bugprone-easily-swappable-parameters,
             -bugprone-implicit-widening-of-multiplication-result,
             -bugprone-narrowing-conversions,
             -bugprone-reserved-identifier,
             -bugprone-signed-char-misuse,
             -bugprone-suspicious-include,
-            -bugprone-too-small-loop-variable,
             -bugprone-unhandled-self-assignment,
             -clang-analyzer-cplusplus.NewDelete,
             -clang-analyzer-cplusplus.NewDeleteLeaks,
@@ -73,8 +71,14 @@ AnalyzeTemporaryDtors: true
 #     we hide the reference implementation in another repository.
 # -modernize-use-trailing-return-type,
 #    At the time of writing (8/22), all of the core BusTub code is in compliance with this. This check only fails
-#    because some GTest file that is generated at build time.  There should be a workaround...?
-#    (The file's name is gmock_main.cc).
+#    because some GTest file (gmock_main.cc) that is generated at build time.  There should be a workaround...?
+# -clang-analyzer-security.insecureAPI.rand, -clang-analyzer-security.insecureAPI.rand, -bugprone-unhandled-self-assignment, 
+# -bugprone-implicit-widening-of-multiplication-result
+#     These have not been investigated yet.
+# -bugprone-reserved-identifier, 
+#     Fails due to use of some __SHORT_FILE__ symbol, originating from very old code.
+# -bugprone-suspicious-include, 
+#     False positive due to GTest code.
 # -bugprone-too-small-loop-variable,
 #     Complains about uint8_t or uint16_t when the limit on the loop is a container's .size() (size_t).
 #     We usually do this when we know the maximum size of the container though, so propose leaving disabled.
@@ -88,8 +92,7 @@ AnalyzeTemporaryDtors: true
 #     We use C-style arrays in page.h, type.h and logger.h. They're a little more ergonomic than std::array. Thoughts?
 # -readability-magic-numbers,
 #     Let's not deal with people doing ridiculous things to hack around this. If it bites them, it bites them.
-# -bugprone-branch-clone, -bugprone-signed-char-misuse, -bugprone-unhandled-self-assignment,
-# -clang-diagnostic-implicit-int-float-conversion, -modernize-use-auto, -modernize-use-trailing-return-type,
-# -readability-convert-member-functions-to-static, -readability-make-member-function-const, -readability-qualified-auto,
-# -readability-redundant-access-specifiers
-#    Not available on clang-8. Disable for forward compatibility with students running modern clang versions.
+# -bugprone-signed-char-misuse, -clang-diagnostic-implicit-int-float-conversion, -readability-make-member-function-const, 
+# -readability-qualified-auto, -readability-redundant-access-specifiers
+#     These were previously disabled for not being available in clang-tidy-8. They are now available on our clang-tidy-12, 
+#     and potentially worth investigating/fixing.

src/include/catalog/column.h

@@ -91,7 +91,6 @@ class Column {
   static auto TypeSize(TypeId type) -> uint8_t {
     switch (type) {
       case TypeId::BOOLEAN:
-        return 1;
       case TypeId::TINYINT:
         return 1;
       case TypeId::SMALLINT:

src/include/execution/executors/aggregation_executor.h

@@ -48,11 +48,8 @@ class SimpleAggregationHashTable {
     for (const auto &agg_type : agg_types_) {
       switch (agg_type) {
         case AggregationType::CountAggregate:
-          // Count starts at zero.
-          values.emplace_back(ValueFactory::GetIntegerValue(0));
-          break;
         case AggregationType::SumAggregate:
-          // Sum starts at zero.
+          // Count/Sum starts at zero.
           values.emplace_back(ValueFactory::GetIntegerValue(0));
           break;
         case AggregationType::MinAggregate: