From 94872ef6daee0f702479d127c9726ecea1d027af Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 18 Jan 2023 12:20:56 -0800 Subject: [PATCH] Change TokenKind's Print overload to a format_provider. (#2534) Fundamentally this `.Print()` is wrong for debug output at present because `.fixed_spelling()` can be empty. It's also inconsistent with other enums to use it. We frequently print tokens for debugging, and it's easy to forget to specify `.name()` there. Diagnostics use formatv, so we can provide a format_provider and address it in one spot that way. It also makes it harder to just forget to do the right thing. --- toolchain/diagnostics/diagnostic_emitter.h | 3 +++ toolchain/lexer/BUILD | 1 - toolchain/lexer/token_kind.h | 21 +++++++++++++----- toolchain/lexer/token_kind_test.cpp | 5 ----- toolchain/lexer/tokenized_buffer.cpp | 22 +++++++------------ .../lexer/tokenized_buffer_test_helpers.h | 7 +++--- toolchain/parser/parse_tree.cpp | 2 +- toolchain/parser/parser.cpp | 8 +++---- .../semantics_parse_tree_handler.cpp | 4 ++-- 9 files changed, 36 insertions(+), 37 deletions(-) diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index 77d70d20db68..3433530b1654 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -168,6 +168,9 @@ struct DiagnosticBase { private: // Handles the cast of llvm::Any to Args types for formatv. + // TODO: Custom formatting can be provided with an format_provider, but that + // affects all formatv calls. Consider replacing formatv with a custom call + // that allows diagnostic-specific formatting. template inline auto FormatFnImpl(const DiagnosticMessage& message, std::index_sequence /*indices*/) const diff --git a/toolchain/lexer/BUILD b/toolchain/lexer/BUILD index d1e6b660b9b6..50733b3be7c5 100644 --- a/toolchain/lexer/BUILD +++ b/toolchain/lexer/BUILD @@ -13,7 +13,6 @@ cc_library( textual_hdrs = ["token_kind.def"], deps = [ "//common:check", - "//common:ostream", "//toolchain/common:enum_base", "@llvm-project//llvm:Support", ], diff --git a/toolchain/lexer/token_kind.h b/toolchain/lexer/token_kind.h index ff1b871e3b0d..a29d964a652f 100644 --- a/toolchain/lexer/token_kind.h +++ b/toolchain/lexer/token_kind.h @@ -7,7 +7,7 @@ #include -#include "common/ostream.h" +#include "llvm/Support/FormatVariadicDetails.h" #include "toolchain/common/enum_base.h" namespace Carbon { @@ -70,11 +70,6 @@ class TokenKind : public CARBON_ENUM_BASE(TokenKind) { } return false; } - - // Override the EnumBase printing to use the fixed spelling rather than the - // name for tokens as this better corresponds to the source code the - // represent. - void Print(llvm::raw_ostream& out) const { out << fixed_spelling(); } }; #define CARBON_TOKEN(TokenName) \ @@ -83,4 +78,18 @@ class TokenKind : public CARBON_ENUM_BASE(TokenKind) { } // namespace Carbon +namespace llvm { + +// We use formatv primarily for diagnostics. In these cases, it's expected that +// the spelling in source code should be used. +template <> +struct format_provider { + static void format(const Carbon::TokenKind& kind, raw_ostream& out, + StringRef /*style*/) { + out << kind.fixed_spelling(); + } +}; + +} // namespace llvm + #endif // CARBON_TOOLCHAIN_LEXER_TOKEN_KIND_H_ diff --git a/toolchain/lexer/token_kind_test.cpp b/toolchain/lexer/token_kind_test.cpp index 0d9fa3eb862a..ba12f7e64577 100644 --- a/toolchain/lexer/token_kind_test.cpp +++ b/toolchain/lexer/token_kind_test.cpp @@ -27,14 +27,12 @@ constexpr llvm::StringLiteral KeywordRegex = "[a-z_]+|Self"; #define CARBON_TOKEN(TokenName) \ TEST(TokenKindTest, TokenName) { \ - EXPECT_EQ(#TokenName, TokenKind::TokenName.name()); \ EXPECT_FALSE(TokenKind::TokenName.is_symbol()); \ EXPECT_FALSE(TokenKind::TokenName.is_keyword()); \ EXPECT_EQ("", TokenKind::TokenName.fixed_spelling()); \ } #define CARBON_SYMBOL_TOKEN(TokenName, Spelling) \ TEST(TokenKindTest, TokenName) { \ - EXPECT_EQ(#TokenName, TokenKind::TokenName.name()); \ EXPECT_TRUE(TokenKind::TokenName.is_symbol()); \ EXPECT_FALSE(TokenKind::TokenName.is_grouping_symbol()); \ EXPECT_FALSE(TokenKind::TokenName.is_opening_symbol()); \ @@ -45,7 +43,6 @@ constexpr llvm::StringLiteral KeywordRegex = "[a-z_]+|Self"; } #define CARBON_OPENING_GROUP_SYMBOL_TOKEN(TokenName, Spelling, ClosingName) \ TEST(TokenKindTest, TokenName) { \ - EXPECT_EQ(#TokenName, TokenKind::TokenName.name()); \ EXPECT_TRUE(TokenKind::TokenName.is_symbol()); \ EXPECT_TRUE(TokenKind::TokenName.is_grouping_symbol()); \ EXPECT_TRUE(TokenKind::TokenName.is_opening_symbol()); \ @@ -57,7 +54,6 @@ constexpr llvm::StringLiteral KeywordRegex = "[a-z_]+|Self"; } #define CARBON_CLOSING_GROUP_SYMBOL_TOKEN(TokenName, Spelling, OpeningName) \ TEST(TokenKindTest, TokenName) { \ - EXPECT_EQ(#TokenName, TokenKind::TokenName.name()); \ EXPECT_TRUE(TokenKind::TokenName.is_symbol()); \ EXPECT_TRUE(TokenKind::TokenName.is_grouping_symbol()); \ EXPECT_FALSE(TokenKind::TokenName.is_opening_symbol()); \ @@ -69,7 +65,6 @@ constexpr llvm::StringLiteral KeywordRegex = "[a-z_]+|Self"; } #define CARBON_KEYWORD_TOKEN(TokenName, Spelling) \ TEST(TokenKindTest, TokenName) { \ - EXPECT_EQ(#TokenName, TokenKind::TokenName.name()); \ EXPECT_FALSE(TokenKind::TokenName.is_symbol()); \ EXPECT_TRUE(TokenKind::TokenName.is_keyword()); \ EXPECT_EQ(Spelling, TokenKind::TokenName.fixed_spelling()); \ diff --git a/toolchain/lexer/tokenized_buffer.cpp b/toolchain/lexer/tokenized_buffer.cpp index f7b448a7ace3..b9f2c8578e49 100644 --- a/toolchain/lexer/tokenized_buffer.cpp +++ b/toolchain/lexer/tokenized_buffer.cpp @@ -652,30 +652,26 @@ auto TokenizedBuffer::GetTokenText(Token token) const -> llvm::StringRef { return llvm::StringRef(); } - CARBON_CHECK(token_info.kind == TokenKind::Identifier) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind == TokenKind::Identifier) << token_info.kind; return GetIdentifierText(token_info.id); } auto TokenizedBuffer::GetIdentifier(Token token) const -> Identifier { const auto& token_info = GetTokenInfo(token); - CARBON_CHECK(token_info.kind == TokenKind::Identifier) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind == TokenKind::Identifier) << token_info.kind; return token_info.id; } auto TokenizedBuffer::GetIntegerLiteral(Token token) const -> const llvm::APInt& { const auto& token_info = GetTokenInfo(token); - CARBON_CHECK(token_info.kind == TokenKind::IntegerLiteral) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind == TokenKind::IntegerLiteral) << token_info.kind; return literal_int_storage_[token_info.literal_index]; } auto TokenizedBuffer::GetRealLiteral(Token token) const -> RealLiteralValue { const auto& token_info = GetTokenInfo(token); - CARBON_CHECK(token_info.kind == TokenKind::RealLiteral) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind == TokenKind::RealLiteral) << token_info.kind; // Note that every real literal is at least three characters long, so we can // safely look at the second character to determine whether we have a @@ -690,16 +686,14 @@ auto TokenizedBuffer::GetRealLiteral(Token token) const -> RealLiteralValue { auto TokenizedBuffer::GetStringLiteral(Token token) const -> llvm::StringRef { const auto& token_info = GetTokenInfo(token); - CARBON_CHECK(token_info.kind == TokenKind::StringLiteral) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind == TokenKind::StringLiteral) << token_info.kind; return literal_string_storage_[token_info.literal_index]; } auto TokenizedBuffer::GetTypeLiteralSize(Token token) const -> const llvm::APInt& { const auto& token_info = GetTokenInfo(token); - CARBON_CHECK(token_info.kind.is_sized_type_literal()) - << token_info.kind.name(); + CARBON_CHECK(token_info.kind.is_sized_type_literal()) << token_info.kind; return literal_int_storage_[token_info.literal_index]; } @@ -707,7 +701,7 @@ auto TokenizedBuffer::GetMatchedClosingToken(Token opening_token) const -> Token { const auto& opening_token_info = GetTokenInfo(opening_token); CARBON_CHECK(opening_token_info.kind.is_opening_symbol()) - << opening_token_info.kind.name(); + << opening_token_info.kind; return opening_token_info.closing_token; } @@ -715,7 +709,7 @@ auto TokenizedBuffer::GetMatchedOpeningToken(Token closing_token) const -> Token { const auto& closing_token_info = GetTokenInfo(closing_token); CARBON_CHECK(closing_token_info.kind.is_closing_symbol()) - << closing_token_info.kind.name(); + << closing_token_info.kind; return closing_token_info.opening_token; } diff --git a/toolchain/lexer/tokenized_buffer_test_helpers.h b/toolchain/lexer/tokenized_buffer_test_helpers.h index c4a570c77ecb..103ae442afaf 100644 --- a/toolchain/lexer/tokenized_buffer_test_helpers.h +++ b/toolchain/lexer/tokenized_buffer_test_helpers.h @@ -28,7 +28,7 @@ namespace Testing { struct ExpectedToken { friend auto operator<<(std::ostream& output, const ExpectedToken& expected) -> std::ostream& { - output << "\ntoken: { kind: '" << expected.kind.name().str() << "'"; + output << "\ntoken: { kind: '" << expected.kind << "'"; if (expected.line != -1) { output << ", line: " << expected.line; } @@ -82,9 +82,8 @@ MATCHER_P(HasTokens, raw_all_expected, "") { TokenKind actual_kind = buffer.GetKind(token); if (actual_kind != expected.kind) { - *result_listener << "\nToken " << index << " is a " - << actual_kind.name().str() << ", expected a " - << expected.kind.name().str() << "."; + *result_listener << "\nToken " << index << " is a " << actual_kind + << ", expected a " << expected.kind << "."; matches = false; } diff --git a/toolchain/parser/parse_tree.cpp b/toolchain/parser/parse_tree.cpp index 8d9f11665a2e..1fcd4475c4ac 100644 --- a/toolchain/parser/parse_tree.cpp +++ b/toolchain/parser/parse_tree.cpp @@ -95,7 +95,7 @@ auto ParseTree::PrintNode(llvm::raw_ostream& output, Node n, int depth, if (preorder) { output << "node_index: " << n << ", "; } - output << "kind: '" << n_impl.kind.name() << "', text: '" + output << "kind: '" << n_impl.kind << "', text: '" << tokens_->GetTokenText(n_impl.token) << "'"; if (n_impl.has_error) { diff --git a/toolchain/parser/parser.cpp b/toolchain/parser/parser.cpp index 37229dbca5cf..d6109af1618d 100644 --- a/toolchain/parser/parser.cpp +++ b/toolchain/parser/parser.cpp @@ -77,8 +77,8 @@ class Parser::PrettyStackTraceParseState : public llvm::PrettyStackTraceEntry { auto line = parser_->tokens_->GetLine(token); output << " @ " << parser_->tokens_->GetLineNumber(line) << ":" << parser_->tokens_->GetColumnNumber(token) << ":" - << " token " << token << " : " - << parser_->tokens_->GetKind(token).name() << "\n"; + << " token " << token << " : " << parser_->tokens_->GetKind(token) + << "\n"; } const Parser* parser_; @@ -97,7 +97,7 @@ Parser::Parser(ParseTree& tree, TokenizedBuffer& tokens, --end_; CARBON_CHECK(tokens_->GetKind(*end_) == TokenKind::EndOfFile) << "TokenizedBuffer should end with EndOfFile, ended with " - << tokens_->GetKind(*end_).name(); + << tokens_->GetKind(*end_); } auto Parser::AddLeafNode(ParseNodeKind kind, TokenizedBuffer::Token token, @@ -164,7 +164,7 @@ auto Parser::ConsumeAndAddLeafNodeIf(TokenKind token_kind, auto Parser::ConsumeChecked(TokenKind kind) -> TokenizedBuffer::Token { CARBON_CHECK(PositionIs(kind)) - << "Required " << kind.name() << ", found " << PositionKind().name(); + << "Required " << kind << ", found " << PositionKind(); return Consume(); } diff --git a/toolchain/semantics/semantics_parse_tree_handler.cpp b/toolchain/semantics/semantics_parse_tree_handler.cpp index 88710c0b8dec..8194a56402ae 100644 --- a/toolchain/semantics/semantics_parse_tree_handler.cpp +++ b/toolchain/semantics/semantics_parse_tree_handler.cpp @@ -350,7 +350,7 @@ auto SemanticsParseTreeHandler::HandleInfixOperator(ParseTree::Node parse_node) parse_node, result_type, lhs_id, rhs_id)); break; default: - CARBON_FATAL() << "Unrecognized token kind: " << token_kind.name(); + CARBON_FATAL() << "Unrecognized token kind: " << token_kind; } } @@ -393,7 +393,7 @@ auto SemanticsParseTreeHandler::HandleLiteral(ParseTree::Node parse_node) break; } default: - CARBON_FATAL() << "Unhandled kind: " << token_kind.name(); + CARBON_FATAL() << "Unhandled kind: " << token_kind; } }