From 62ae0e08cfe2a86d1d359abdc0acecfe4cc6929d Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 8 Mar 2021 15:17:21 -0800 Subject: [PATCH] Merge type of diagnostic and its substitutions. (#366) --- diagnostics/diagnostic_emitter.h | 27 +++++++---------- diagnostics/diagnostic_emitter_test.cpp | 36 ++++++++++------------ lexer/numeric_literal.cpp | 40 +++++++++++-------------- 3 files changed, 44 insertions(+), 59 deletions(-) diff --git a/diagnostics/diagnostic_emitter.h b/diagnostics/diagnostic_emitter.h index 704f3990667a..9e6a926191f2 100644 --- a/diagnostics/diagnostic_emitter.h +++ b/diagnostics/diagnostic_emitter.h @@ -37,30 +37,27 @@ class DiagnosticEmitter { : callback_(std::move(callback)) {} ~DiagnosticEmitter() = default; - // Emits an error unconditionally after applying the provided `substitutions`. + // Emits an error unconditionally. template - void EmitError(typename DiagnosticT::Substitutions substitutions) { - callback_({.short_name = DiagnosticT::ShortName, - .message = DiagnosticT::Format(substitutions)}); + auto EmitError(DiagnosticT diag) -> void { + callback_({.short_name = DiagnosticT::ShortName, .message = diag.Format()}); } - // Emits an error unconditionally when there are no substitutions. + // Emits a stateless error unconditionally. template - std::enable_if_t> - EmitError() { + auto EmitError() -> std::enable_if_t> { EmitError({}); } // Emits a warning if `F` returns true. `F` may or may not be called if the // warning is disabled. template - void EmitWarningIf( - llvm::function_ref f) { + auto EmitWarningIf(llvm::function_ref f) -> void { // TODO(kfm): check if this warning is enabled - typename DiagnosticT::Substitutions substitutions; - if (f(substitutions)) { - callback_({.short_name = DiagnosticT::ShortName, - .message = DiagnosticT::Format(substitutions)}); + DiagnosticT diag; + if (f(diag)) { + callback_( + {.short_name = DiagnosticT::ShortName, .message = diag.Format()}); } } @@ -83,9 +80,7 @@ inline auto NullDiagnosticEmitter() -> DiagnosticEmitter& { template struct SimpleDiagnostic { struct Substitutions {}; - static auto Format(const Substitutions&) -> std::string { - return Derived::Message.str(); - } + static auto Format() -> std::string { return Derived::Message.str(); } }; } // namespace Carbon diff --git a/diagnostics/diagnostic_emitter_test.cpp b/diagnostics/diagnostic_emitter_test.cpp index f8319ba2ecb6..e09008c3bfc8 100644 --- a/diagnostics/diagnostic_emitter_test.cpp +++ b/diagnostics/diagnostic_emitter_test.cpp @@ -21,15 +21,14 @@ struct FakeDiagnostic { // selection of the message. static constexpr llvm::StringLiteral Message = "{0}"; - struct Substitutions { - std::string message; - }; - static auto Format(const Substitutions& substitutions) -> std::string { + std::string message; + + auto Format() -> std::string { // Work around a bug in Clang's unused const variable warning by marking it // used here with a no-op. static_cast(ShortName); - return llvm::formatv(Message.data(), substitutions.message).str(); + return llvm::formatv(Message.data(), message).str(); } }; @@ -55,21 +54,18 @@ TEST(DiagTest, EmitWarnings) { reported.push_back(diagnostic.message); }); - emitter.EmitWarningIf( - [](FakeDiagnostic::Substitutions& diagnostic) { - diagnostic.message = "M1"; - return true; - }); - emitter.EmitWarningIf( - [](FakeDiagnostic::Substitutions& diagnostic) { - diagnostic.message = "M2"; - return false; - }); - emitter.EmitWarningIf( - [](FakeDiagnostic::Substitutions& diagnostic) { - diagnostic.message = "M3"; - return true; - }); + emitter.EmitWarningIf([](FakeDiagnostic& diagnostic) { + diagnostic.message = "M1"; + return true; + }); + emitter.EmitWarningIf([](FakeDiagnostic& diagnostic) { + diagnostic.message = "M2"; + return false; + }); + emitter.EmitWarningIf([](FakeDiagnostic& diagnostic) { + diagnostic.message = "M3"; + return true; + }); EXPECT_THAT(reported, ElementsAre("M1", "M3")); } diff --git a/lexer/numeric_literal.cpp b/lexer/numeric_literal.cpp index 3c513292ba52..e46ca79b7065 100644 --- a/lexer/numeric_literal.cpp +++ b/lexer/numeric_literal.cpp @@ -21,16 +21,13 @@ struct EmptyDigitSequence : SimpleDiagnostic { struct InvalidDigit { static constexpr llvm::StringLiteral ShortName = "syntax-invalid-number"; - struct Substitutions { - char digit; - int radix; - }; - static auto Format(const Substitutions& subst) -> std::string { - return llvm::formatv("Invalid digit '{0}' in {1} numeric literal.", - subst.digit, - (subst.radix == 2 ? "binary" - : subst.radix == 16 ? "hexadecimal" - : "decimal")) + char digit; + int radix; + + auto Format() -> std::string { + return llvm::formatv("Invalid digit '{0}' in {1} numeric literal.", digit, + (radix == 2 ? "binary" + : radix == 16 ? "hexadecimal" : "decimal")) .str(); } }; @@ -45,16 +42,15 @@ struct IrregularDigitSeparators { static constexpr llvm::StringLiteral ShortName = "syntax-irregular-digit-separators"; - struct Substitutions { - int radix; - }; - static auto Format(const Substitutions& subst) -> std::string { - assert((subst.radix == 10 || subst.radix == 16) && "unexpected radix"); + int radix; + + auto Format() -> std::string { + assert((radix == 10 || radix == 16) && "unexpected radix"); return llvm::formatv( "Digit separators in {0} number should appear every {1} " "characters from the right.", - (subst.radix == 10 ? "decimal" : "hexadecimal"), - (subst.radix == 10 ? "3" : "4")) + (radix == 10 ? "decimal" : "hexadecimal"), + (radix == 10 ? "3" : "4")) .str(); } }; @@ -74,12 +70,10 @@ struct BinaryRealLiteral : SimpleDiagnostic { struct WrongRealLiteralExponent { static constexpr llvm::StringLiteral ShortName = "syntax-invalid-number"; - struct Substitutions { - char expected; - }; - static auto Format(const Substitutions& subst) -> std::string { - return llvm::formatv("Expected '{0}' to introduce exponent.", - subst.expected) + char expected; + + auto Format() -> std::string { + return llvm::formatv("Expected '{0}' to introduce exponent.", expected) .str(); } };