Addressing comments

Author: jonmeow

toolchain/check/convert.cpp

@@ -416,9 +416,9 @@ static auto ConvertStructToStructOrClass(Context& context,
         StructInitElementCountMismatch, Error,
         "cannot initialize {0:class|struct} with {1} field(s) from struct "
         "with {2} field(s).",
-        FormatBool, size_t, size_t);
+        BoolAsSelect, size_t, size_t);
     context.emitter().Emit(value_loc_id, StructInitElementCountMismatch,
-                           {.value = ToClass}, dest_elem_fields.size(),
+                           ToClass, dest_elem_fields.size(),
                            src_elem_fields.size());
     return SemIR::InstId::BuiltinError;
   }
@@ -1163,9 +1163,8 @@ static auto ConvertSelf(Context& context, SemIR::LocId call_loc_id,
         CARBON_DIAGNOSTIC(InCallToFunctionSelf, Note,
                           "initializing `{0:addr self|self}` parameter of "
                           "method declared here",
-                          FormatBool);
-        builder.Note(self_param_id, InCallToFunctionSelf,
-                     {.value = addr_pattern});
+                          BoolAsSelect);
+        builder.Note(self_param_id, InCallToFunctionSelf, addr_pattern);
       });
 
   return CallerPatternMatch(context, callee_specific_id, self_param_id,

toolchain/check/eval.cpp

@@ -751,11 +751,11 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLoc loc,
         CARBON_DIAGNOSTIC(
             CompileTimeShiftOutOfRange, Error,
             "shift distance not in range [0, {0}) in {1} {2:<<|>>} {3}",
-            unsigned, TypedInt, FormatBool, TypedInt);
+            unsigned, TypedInt, BoolAsSelect, TypedInt);
         context.emitter().Emit(
             loc, CompileTimeShiftOutOfRange, lhs_val.getBitWidth(),
             {.type = lhs.type_id, .value = lhs_val},
-            {.value = builtin_kind == SemIR::BuiltinFunctionKind::IntLeftShift},
+            builtin_kind == SemIR::BuiltinFunctionKind::IntLeftShift,
             {.type = rhs.type_id, .value = rhs_val});
         // TODO: Is it useful to recover by returning 0 or -1?
         return SemIR::ConstantId::Error;

toolchain/check/handle_binding_pattern.cpp

@@ -106,18 +106,18 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
           [&] {
             CARBON_DIAGNOSTIC(IncompleteTypeInVarDecl, Error,
                               "{0:field|variable} has incomplete type {1}",
-                              FormatBool, SemIR::TypeId);
-            return context.emitter().Build(
-                type_node, IncompleteTypeInVarDecl,
-                {.value = parent_class_decl.has_value()}, cast_type_id);
+                              BoolAsSelect, SemIR::TypeId);
+            return context.emitter().Build(type_node, IncompleteTypeInVarDecl,
+                                           parent_class_decl.has_value(),
+                                           cast_type_id);
           },
           [&] {
             CARBON_DIAGNOSTIC(AbstractTypeInVarDecl, Error,
                               "{0:field|variable} has abstract type {1}",
-                              FormatBool, SemIR::TypeId);
-            return context.emitter().Build(
-                type_node, AbstractTypeInVarDecl,
-                {.value = parent_class_decl.has_value()}, cast_type_id);
+                              BoolAsSelect, SemIR::TypeId);
+            return context.emitter().Build(type_node, AbstractTypeInVarDecl,
+                                           parent_class_decl.has_value(),
+                                           cast_type_id);
           });
       if (parent_class_decl) {
         CARBON_CHECK(context_node_kind == Parse::NodeKind::VariableIntroducer,

toolchain/check/merge.cpp

@@ -12,29 +12,6 @@
 
 namespace Carbon::Check {
 
-// Explicit or implicit parameters. Used in diagnostics.
-enum class ParamKind : uint8_t {
-  Explicit,
-  Implicit,
-};
-
-}  // namespace Carbon::Check
-
-// `ParamKind` is formatted in diagnostics as `{0}parameters`, and we only add
-// text for implicit paramseters.
-template <>
-struct llvm::format_provider<Carbon::Check::ParamKind> {
-  using ParamKind = Carbon::Check::ParamKind;
-  static void format(const ParamKind& kind, raw_ostream& out,
-                     StringRef /*style*/) {
-    if (kind == ParamKind::Implicit) {
-      out << "implicit ";
-    }
-  }
-};
-
-namespace Carbon::Check {
-
 CARBON_DIAGNOSTIC(RedeclPrevDecl, Note, "previously declared here");
 
 // Diagnoses a redeclaration which is redundant.
@@ -217,7 +194,7 @@ static auto EntityHasParamError(Context& context, const DeclParams& info)
 
 // Returns false if a param differs for a redeclaration. The caller is expected
 // to provide a diagnostic.
-static auto CheckRedeclParam(Context& context, ParamKind param_kind,
+static auto CheckRedeclParam(Context& context, bool is_implicit_param,
                              int32_t param_index,
                              SemIR::InstId new_param_pattern_id,
                              SemIR::InstId prev_param_pattern_id,
@@ -231,15 +208,16 @@ static auto CheckRedeclParam(Context& context, ParamKind param_kind,
       return;
     }
     CARBON_DIAGNOSTIC(RedeclParamDiffers, Error,
-                      "redeclaration differs at {0}parameter {1}", ParamKind,
-                      int32_t);
-    CARBON_DIAGNOSTIC(RedeclParamPrevious, Note,
-                      "previous declaration's corresponding {0}parameter here",
-                      ParamKind);
+                      "redeclaration differs at {0:implicit |}parameter {1}",
+                      BoolAsSelect, int32_t);
+    CARBON_DIAGNOSTIC(
+        RedeclParamPrevious, Note,
+        "previous declaration's corresponding {0:implicit |}parameter here",
+        BoolAsSelect);
     context.emitter()
-        .Build(new_param_pattern_id, RedeclParamDiffers, param_kind,
+        .Build(new_param_pattern_id, RedeclParamDiffers, is_implicit_param,
                param_index + 1)
-        .Note(prev_param_pattern_id, RedeclParamPrevious, param_kind)
+        .Note(prev_param_pattern_id, RedeclParamPrevious, is_implicit_param)
         .Emit();
   };
 
@@ -291,7 +269,7 @@ static auto CheckRedeclParams(Context& context, SemIRLoc new_decl_loc,
                               SemIR::InstBlockId new_param_patterns_id,
                               SemIRLoc prev_decl_loc,
                               SemIR::InstBlockId prev_param_patterns_id,
-                              ParamKind param_kind,
+                              bool is_implicit_param,
                               SemIR::SpecificId prev_specific_id, bool diagnose)
     -> bool {
   // This will often occur for empty params.
@@ -304,18 +282,19 @@ static auto CheckRedeclParams(Context& context, SemIRLoc new_decl_loc,
     if (!diagnose) {
       return false;
     }
-    CARBON_DIAGNOSTIC(
-        RedeclParamListDiffers, Error,
-        "redeclaration differs because of {1:'|missing '}{0}parameter list",
-        ParamKind, FormatBool);
+    CARBON_DIAGNOSTIC(RedeclParamListDiffers, Error,
+                      "redeclaration differs because of "
+                      "{1:'|missing '}{0:implicit |}parameter list",
+                      BoolAsSelect, BoolAsSelect);
     CARBON_DIAGNOSTIC(RedeclParamListPrevious, Note,
-                      "previously declared {1:with|without} {0}parameter list",
-                      ParamKind, FormatBool);
+                      "previously declared "
+                      "{1:with|without} {0:implicit |}parameter list",
+                      BoolAsSelect, BoolAsSelect);
     context.emitter()
-        .Build(new_decl_loc, RedeclParamListDiffers, param_kind,
-               {.value = new_param_patterns_id.is_valid()})
-        .Note(prev_decl_loc, RedeclParamListPrevious, param_kind,
-              {.value = prev_param_patterns_id.is_valid()})
+        .Build(new_decl_loc, RedeclParamListDiffers, is_implicit_param,
+               new_param_patterns_id.is_valid())
+        .Note(prev_decl_loc, RedeclParamListPrevious, is_implicit_param,
+              prev_param_patterns_id.is_valid())
         .Emit();
     return false;
   }
@@ -332,23 +311,25 @@ static auto CheckRedeclParams(Context& context, SemIRLoc new_decl_loc,
     }
     CARBON_DIAGNOSTIC(
         RedeclParamCountDiffers, Error,
-        "redeclaration differs because of {0}parameter count of {1}", ParamKind,
-        int32_t);
-    CARBON_DIAGNOSTIC(RedeclParamCountPrevious, Note,
-                      "previously declared with {0}parameter count of {1}",
-                      ParamKind, int32_t);
+        "redeclaration differs because of {0:implicit |}parameter count of {1}",
+        BoolAsSelect, int32_t);
+    CARBON_DIAGNOSTIC(
+        RedeclParamCountPrevious, Note,
+        "previously declared with {0:implicit |}parameter count of {1}",
+        BoolAsSelect, int32_t);
     context.emitter()
-        .Build(new_decl_loc, RedeclParamCountDiffers, param_kind,
+        .Build(new_decl_loc, RedeclParamCountDiffers, is_implicit_param,
                new_param_pattern_ids.size())
-        .Note(prev_decl_loc, RedeclParamCountPrevious, param_kind,
+        .Note(prev_decl_loc, RedeclParamCountPrevious, is_implicit_param,
               prev_param_pattern_ids.size())
         .Emit();
     return false;
   }
   for (auto [index, new_param_pattern_id, prev_param_pattern_id] :
        llvm::enumerate(new_param_pattern_ids, prev_param_pattern_ids)) {
-    if (!CheckRedeclParam(context, param_kind, index, new_param_pattern_id,
-                          prev_param_pattern_id, prev_specific_id, diagnose)) {
+    if (!CheckRedeclParam(context, is_implicit_param, index,
+                          new_param_pattern_id, prev_param_pattern_id,
+                          prev_specific_id, diagnose)) {
       return false;
     }
   }
@@ -434,15 +415,16 @@ auto CheckRedeclParamsMatch(Context& context, const DeclParams& new_entity,
       EntityHasParamError(context, prev_entity)) {
     return false;
   }
-  if (!CheckRedeclParams(context, new_entity.loc,
-                         new_entity.implicit_param_patterns_id, prev_entity.loc,
-                         prev_entity.implicit_param_patterns_id,
-                         ParamKind::Implicit, prev_specific_id, diagnose)) {
+  if (!CheckRedeclParams(
+          context, new_entity.loc, new_entity.implicit_param_patterns_id,
+          prev_entity.loc, prev_entity.implicit_param_patterns_id,
+          /*is_implicit_param=*/true, prev_specific_id, diagnose)) {
     return false;
   }
   if (!CheckRedeclParams(context, new_entity.loc, new_entity.param_patterns_id,
                          prev_entity.loc, prev_entity.param_patterns_id,
-                         ParamKind::Explicit, prev_specific_id, diagnose)) {
+                         /*is_implicit_param=*/false, prev_specific_id,
+                         diagnose)) {
     return false;
   }
   if (check_syntax &&

toolchain/diagnostics/BUILD

@@ -59,6 +59,7 @@ cc_library(
 
 cc_library(
     name = "format_providers",
+    srcs = ["format_providers.cpp"],
     hdrs = ["format_providers.h"],
     deps = [
         "//common:check",

toolchain/diagnostics/format_providers.cpp

@@ -0,0 +1,92 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "toolchain/diagnostics/format_providers.h"
+
+#include "common/check.h"
+#include "llvm/ADT/StringExtras.h"
+
+auto llvm::format_provider<Carbon::BoolAsSelect>::format(
+    const Carbon::BoolAsSelect& wrapper, raw_ostream& out, StringRef style)
+    -> void {
+  if (style.empty()) {
+    llvm::format_provider<bool>::format(wrapper.value, out, style);
+    return;
+  }
+
+  // Remove wrapping quotes if present.
+  if (style.starts_with('\'') && style.ends_with('\'')) {
+    style = style.drop_front().drop_back();
+  }
+
+  auto sep = style.find('|');
+  CARBON_CHECK(
+      sep != llvm::StringRef::npos,
+      "BoolAsSelect requires a `|` separating true and false results: `{0}`",
+      style);
+  CARBON_CHECK(style.find('|', sep + 1) == llvm::StringRef::npos,
+               "BoolAsSelect only allows one `|`: `{0}`", style);
+
+  if (wrapper.value) {
+    out << style.take_front(sep);
+  } else {
+    out << style.drop_front(sep + 1);
+  }
+}
+
+auto llvm::format_provider<Carbon::IntAsSelect>::format(
+    const Carbon::IntAsSelect& wrapper, raw_ostream& out, StringRef style)
+    -> void {
+  if (style.empty()) {
+    llvm::format_provider<int>::format(wrapper.value, out, style);
+    return;
+  }
+
+  // Remove wrapping quotes if present.
+  if (style.starts_with('\'') && style.ends_with('\'')) {
+    style = style.drop_front().drop_back();
+  }
+
+  auto cursor = style;
+  while (!cursor.empty()) {
+    auto case_sep = cursor.find("|");
+    auto token = cursor.substr(0, case_sep);
+    if (case_sep == llvm::StringRef::npos) {
+      cursor = llvm::StringRef();
+    } else {
+      cursor = cursor.drop_front(case_sep + 1);
+    }
+
+    auto pair_sep = token.find(':');
+    CARBON_CHECK(pair_sep != llvm::StringRef::npos,
+                 "IntAsSelect requires a `:` separating each comparison and "
+                 "output string: `{0}`",
+                 style);
+
+    auto comp = token.take_front(pair_sep);
+    auto output_string = token.drop_front(pair_sep + 1);
+
+    if (comp.empty()) {
+      // Default case.
+      CARBON_CHECK(cursor.empty(),
+                   "IntAsSelect requires the default case be last: `{0}`",
+                   style);
+      out << output_string;
+      return;
+    } else if (comp.consume_front("=")) {
+      // Equality comparison.
+      int value;
+      CARBON_CHECK(to_integer(comp, value),
+                   "IntAsSelect has invalid value in comparison: `{0}`", style);
+      if (value == wrapper.value) {
+        out << output_string;
+        return;
+      }
+    } else {
+      CARBON_FATAL("IntAsSelect has unrecognized comparison: `{0}`", style);
+    }
+  }
+
+  CARBON_FATAL("IntAsSelect doesn't handle `{0}`: `{1}`", wrapper.value, style);
+}