From c3d951599d4c227ed1bd41cd3349a101be04daad Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Tue, 8 Dec 2020 15:35:26 -0800 Subject: [PATCH] Try to move towards the Google style guide declaration order. (#221) This patch tries to make everything adhere to the Google declaration order. This is tricky as there doesn't appear to be a `clang-tidy` check that helps us here at all. One of the complex cases are the `enum`-wrapping classes we use. These need to have a *public* conversion operator to the nested `enum` type to support `switch` statements and the like. However, this makes it impossible to fully respect the Google declaration order. We need to define the enum type before the public API in order to use it in contexts like the conversion operator. Even defining out-of-line won't help avoid this. It is weird to have a type in the public API that is private, but again this is only intended to be used for implicit conversions within a `switch` statement or a `case` label. In one case, we had the non-conforming order of declaration. I've added a comment to explain why there. In the other case, the conversion operator is actually *private* rather than public, which doesn't actually work in practice. I've made this public and moved the declaration order to match with a matching comment. --- lexer/token_kind.h | 2 ++ lexer/tokenized_buffer.h | 8 ++++---- lexer/tokenized_buffer_test_helpers.h | 14 +++++++------- parser/parse_node_kind.h | 18 ++++++++++-------- parser/parse_tree.h | 18 +++++++++--------- parser/parser_impl.h | 12 ++++++------ 6 files changed, 38 insertions(+), 34 deletions(-) diff --git a/lexer/token_kind.h b/lexer/token_kind.h index 86070ce35c5c..4e76ad8d6f3c 100644 --- a/lexer/token_kind.h +++ b/lexer/token_kind.h @@ -13,6 +13,8 @@ namespace Carbon { class TokenKind { + // Note that this must be declared earlier in the class so that its type can + // be used, for example in the conversion operator. enum class KindEnum : int8_t { #define CARBON_TOKEN(TokenName) TokenName, #include "lexer/token_registry.def" diff --git a/lexer/tokenized_buffer.h b/lexer/tokenized_buffer.h index fa8c18521913..2ca8233a75db 100644 --- a/lexer/tokenized_buffer.h +++ b/lexer/tokenized_buffer.h @@ -274,15 +274,15 @@ class TokenizedBuffer { // Specifies minimum widths to use when printing a token's fields via // `printToken`. struct PrintWidths { + // Widens `this` to the maximum of `this` and `new_width` for each + // dimension. + void Widen(const PrintWidths& new_width); + int index; int kind; int column; int line; int indent; - - // Widens `this` to the maximum of `this` and `new_width` for each - // dimension. - void Widen(const PrintWidths& new_width); }; struct TokenInfo { diff --git a/lexer/tokenized_buffer_test_helpers.h b/lexer/tokenized_buffer_test_helpers.h index ebc099472f13..039fd8fd470b 100644 --- a/lexer/tokenized_buffer_test_helpers.h +++ b/lexer/tokenized_buffer_test_helpers.h @@ -24,13 +24,6 @@ inline void PrintTo(const TokenizedBuffer& buffer, std::ostream* output) { namespace Testing { struct ExpectedToken { - TokenKind kind; - int line = -1; - int column = -1; - int indent_column = -1; - bool recovery = false; - llvm::StringRef text = ""; - friend auto operator<<(std::ostream& output, const ExpectedToken& expected) -> std::ostream& { output << "\ntoken: { kind: '" << expected.kind.Name().str(); @@ -52,6 +45,13 @@ struct ExpectedToken { output << " }"; return output; } + + TokenKind kind; + int line = -1; + int column = -1; + int indent_column = -1; + bool recovery = false; + llvm::StringRef text = ""; }; // TODO: Consider rewriting this into a `TokenEq` matcher which is used inside diff --git a/parser/parse_node_kind.h b/parser/parse_node_kind.h index debcd67421ad..c9c1eec4a47f 100644 --- a/parser/parse_node_kind.h +++ b/parser/parse_node_kind.h @@ -24,6 +24,13 @@ namespace Carbon { // member functions. These instances are designed specifically to be usable in // `case` labels of `switch` statements just like an enumerator would. class ParseNodeKind { + // Note that this must be declared earlier in the class so that its type can + // be used, for example in the conversion operator. + enum class KindEnum : uint8_t { +#define CARBON_PARSE_NODE_KIND(Name) Name, +#include "parser/parse_node_kind.def" + }; + public: // The formatting for this macro is weird due to a `clang-format` bug. See // https://bugs.llvm.org/show_bug.cgi?id=48320 for details. @@ -47,20 +54,15 @@ class ParseNodeKind { // Gets a friendly name for the token for logging or debugging. [[nodiscard]] auto GetName() const -> llvm::StringRef; - private: - enum class KindEnum : uint8_t { -#define CARBON_PARSE_NODE_KIND(Name) Name, -#include "parser/parse_node_kind.def" - }; - - constexpr explicit ParseNodeKind(KindEnum k) : kind(k) {} - // Enable conversion to our private enum, including in a `constexpr` context, // to enable usage in `switch` and `case`. The enum remains private and // nothing else should be using this. // NOLINTNEXTLINE(google-explicit-constructor) constexpr operator KindEnum() const { return kind; } + private: + constexpr explicit ParseNodeKind(KindEnum k) : kind(k) {} + KindEnum kind; }; diff --git a/parser/parse_tree.h b/parser/parse_tree.h index 8a601fcdcbb7..edccd14bd8d3 100644 --- a/parser/parse_tree.h +++ b/parser/parse_tree.h @@ -140,6 +140,10 @@ class ParseTree { // The in-memory representation of data used for a particular node in the // tree. struct NodeImpl { + explicit NodeImpl(ParseNodeKind k, TokenizedBuffer::Token t, + int subtree_size_arg) + : kind(k), token(t), subtree_size(subtree_size_arg) {} + // The kind of this node. Note that this is only a single byte. ParseNodeKind kind; @@ -179,10 +183,6 @@ class ParseTree { // This field should always be a positive integer as at least this node is // part of its subtree. int32_t subtree_size; - - explicit NodeImpl(ParseNodeKind k, TokenizedBuffer::Token t, - int subtree_size_arg) - : kind(k), token(t), subtree_size(subtree_size_arg) {} }; static_assert(sizeof(NodeImpl) == 12, @@ -301,9 +301,9 @@ class ParseTree::PostorderIterator private: friend class ParseTree; - Node node; - explicit PostorderIterator(Node n) : node(n) {} + + Node node; }; // A forward iterator across the silbings at a particular level in the parse @@ -343,12 +343,12 @@ class ParseTree::SiblingIterator private: friend class ParseTree; + explicit SiblingIterator(const ParseTree& tree_arg, Node n) + : tree(&tree_arg), node(n) {} + const ParseTree* tree; Node node; - - explicit SiblingIterator(const ParseTree& tree_arg, Node n) - : tree(&tree_arg), node(n) {} }; } // namespace Carbon diff --git a/parser/parser_impl.h b/parser/parser_impl.h index 95da44e19c19..13729c8d5e14 100644 --- a/parser/parser_impl.h +++ b/parser/parser_impl.h @@ -25,12 +25,6 @@ class ParseTree::Parser { private: struct SubtreeStart; - ParseTree& tree; - TokenizedBuffer& tokens; - - TokenizedBuffer::TokenIterator position; - TokenizedBuffer::TokenIterator end; - explicit Parser(ParseTree& tree_arg, TokenizedBuffer& tokens_arg) : tree(tree_arg), tokens(tokens_arg), @@ -127,6 +121,12 @@ class ParseTree::Parser { // skipping errors, can be parsed, it is returned. There may be parse errors // even when a node is returned. auto ParseDeclaration() -> llvm::Optional; + + ParseTree& tree; + TokenizedBuffer& tokens; + + TokenizedBuffer::TokenIterator position; + TokenizedBuffer::TokenIterator end; }; } // namespace Carbon