Disable bugprone-macro-parentheses and let clang-format insert braces. (#3825)

-
[bugprone-macro-parentheses](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/macro-parentheses.html)
-- this is just a false positive issue, I don't think it's helping us
catch bugs.
-
[InsertBraces](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces)
-- although there's a warning about this creating issues due to
incomplete semantic information, it seems to be happy with our code, and
allows clang-format to fix something that clang-tidy would otherwise
warn about.

.clang-format

@@ -10,6 +10,7 @@ AllowShortLoopsOnASingleLine: 'false'
 DerivePointerAlignment: 'false'
 ExperimentalAutoDetectBinPacking: 'false'
 FixNamespaceComments: 'true'
+InsertBraces: 'true'
 PointerAlignment: Left
 # We abuse control macros for formatting other kinds of macros.
 SpaceBeforeParens: ControlStatementsExceptControlMacros

.clang-tidy

@@ -10,14 +10,17 @@ WarningsAsErrors: '*'
 
 # - bugprone-exception-escape finds issues like out-of-memory in main(). We
 #   don't use exceptions, so it's unlikely to find real issues.
+# - bugprone-macro-parentheses has false positives in places such as using an
+#   argument to declare a name, which cannot have parentheses. For our limited
+#   use of macros, this is a common conflict.
+# - bugprone-switch-missing-default-case has false positives for `enum_base.h`.
+#   Clang's built-in switch warnings cover most of our risk of bugs here.
 # - bugprone-unchecked-optional-access in clang-tidy 16 has false positives on
 #   code like:
 #     while (auto name_ref = insts().Get(inst_id).TryAs<SemIR::NameRef>()) {
 #       inst_id = name_ref->value_id;
 #                 ^ unchecked access to optional value
 #     }
-# - bugprone-switch-missing-default-case has false positives for `enum_base.h`.
-#   Clang's built-in switch warnings cover most of our risk of bugs here.
 # - google-readability-function-size overlaps with readability-function-size.
 # - modernize-use-designated-initializers is disabled because it fires on
 #   creation of SemIR typed insts, for which we do not currently want to use
@@ -28,9 +31,10 @@ WarningsAsErrors: '*'
 #   modernize-pass-by-value.
 Checks:
   -*, bugprone-*, -bugprone-branch-clone, -bugprone-easily-swappable-parameters,
-  -bugprone-exception-escape, -bugprone-narrowing-conversions,
-  -bugprone-switch-missing-default-case, -bugprone-unchecked-optional-access,
-  google-*, -google-readability-function-size, -google-readability-todo,
+  -bugprone-exception-escape, -bugprone-macro-parentheses,
+  -bugprone-narrowing-conversions, -bugprone-switch-missing-default-case,
+  -bugprone-unchecked-optional-access, google-*,
+  -google-readability-function-size, -google-readability-todo,
   misc-definitions-in-headers, misc-misplaced-const, misc-redundant-expression,
   misc-static-assert, misc-unconventional-assign-operator,
   misc-uniqueptr-reset-release, misc-unused-*, modernize-*,

common/enum_base.h

@@ -173,7 +173,6 @@ class EnumBase : public Printable<DerivedT> {
 // For use when multiple enums use the same list of names.
 #define CARBON_DEFINE_RAW_ENUM_CLASS_NO_NAMES(EnumClassName, UnderlyingType) \
   namespace Internal {                                                       \
-  /* NOLINTNEXTLINE(bugprone-macro-parentheses) */                           \
   enum class EnumClassName##RawEnum : UnderlyingType;                        \
   }                                                                          \
   enum class Internal::EnumClassName##RawEnum : UnderlyingType

common/error.h

@@ -179,10 +179,9 @@ class ErrorBuilder {
 // argument separator.
 #define CARBON_PROTECT_COMMAS(...) __VA_ARGS__
 
-#define CARBON_RETURN_IF_ERROR_IMPL(unique_name, expr)                    \
-  if (auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \
-      !(unique_name).ok()) {                                              \
-    return std::move(unique_name).error();                                \
+#define CARBON_RETURN_IF_ERROR_IMPL(unique_name, expr)  \
+  if (auto unique_name = (expr); !(unique_name).ok()) { \
+    return std::move(unique_name).error();              \
   }
 
 #define CARBON_RETURN_IF_ERROR(expr)                                    \
@@ -190,12 +189,12 @@ class ErrorBuilder {
       CARBON_MAKE_UNIQUE_NAME(_llvm_error_line, __LINE__, __COUNTER__), \
       CARBON_PROTECT_COMMAS(expr))
 
-#define CARBON_ASSIGN_OR_RETURN_IMPL(unique_name, var, expr)          \
-  auto unique_name = (expr); /* NOLINT(bugprone-macro-parentheses) */ \
-  if (!(unique_name).ok()) {                                          \
-    return std::move(unique_name).error();                            \
-  }                                                                   \
-  var = std::move(*(unique_name)); /* NOLINT(bugprone-macro-parentheses) */
+#define CARBON_ASSIGN_OR_RETURN_IMPL(unique_name, var, expr) \
+  auto unique_name = (expr);                                 \
+  if (!(unique_name).ok()) {                                 \
+    return std::move(unique_name).error();                   \
+  }                                                          \
+  var = std::move(*(unique_name));
 
 #define CARBON_ASSIGN_OR_RETURN(var, expr)                                 \
   CARBON_ASSIGN_OR_RETURN_IMPL(                                            \

toolchain/base/kind_switch.h

@@ -78,8 +78,6 @@ auto Cast(ValueT&& kind_switch_value) -> auto {
 // This uses `if` to scope the variable, and provides a dangling `else` in order
 // to prevent accidental `else` use. The label allows `:` to follow the macro
 // name, making it look more like a typical `case`.
-//
-// NOLINTBEGIN(bugprone-macro-parentheses)
 #define CARBON_KIND(typed_variable_declaration)                                \
   ::Carbon::Internal::Kind::ForCase<                                           \
       decltype([]([[maybe_unused]] typed_variable_declaration) {})>()          \
@@ -88,6 +86,5 @@ auto Cast(ValueT&& kind_switch_value) -> auto {
                 carbon_internal_kind_switch_value);                            \
             false) {}                                                          \
   else [[maybe_unused]] CARBON_INTERNAL_KIND_LABEL(__LINE__)
-// NOLINTEND(bugprone-macro-parentheses)
 
 #endif  // CARBON_TOOLCHAIN_BASE_KIND_SWITCH_H_

toolchain/parse/node_kind.h

@@ -95,7 +95,6 @@ class NodeKind : public CARBON_ENUM_BASE(NodeKind) {
 #include "toolchain/parse/node_kind.def"
 
 constexpr int NodeKind::ValidCount = 0
-// NOLINTNEXTLINE(bugprone-macro-parentheses)
 #define CARBON_PARSE_NODE_KIND(Name) +1
 #include "toolchain/parse/node_kind.def"
     ;