From 97ed697386568406d42bdc6dca7b4394e8bc1bf8 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 18 Nov 2022 14:07:01 -0800 Subject: [PATCH] For flyweights, shift from llvm::SmallVector to a type that enforces index types. (#2398) The intent here is to reduce use of vanilla `int32_t` without a clear indicator of what it's referencing, and to more tightly link references with the underlying types they reference into. The type name DataIndex doesn't feel great, but I was kind of floundering for a better name. --- toolchain/common/BUILD | 9 ++++ toolchain/common/index_base.h | 48 +++++++++++++++++ toolchain/lexer/BUILD | 1 + toolchain/lexer/tokenized_buffer.cpp | 22 ++++---- toolchain/lexer/tokenized_buffer.h | 80 +++++++--------------------- 5 files changed, 87 insertions(+), 73 deletions(-) create mode 100644 toolchain/common/index_base.h diff --git a/toolchain/common/BUILD b/toolchain/common/BUILD index c1fc155b6da0..55aed41553ba 100644 --- a/toolchain/common/BUILD +++ b/toolchain/common/BUILD @@ -4,6 +4,15 @@ package(default_visibility = ["//visibility:public"]) +cc_library( + name = "index_base", + hdrs = ["index_base.h"], + deps = [ + "//common:ostream", + "@llvm-project//llvm:Support", + ], +) + cc_library( name = "yaml_test_helpers", testonly = 1, diff --git a/toolchain/common/index_base.h b/toolchain/common/index_base.h new file mode 100644 index 000000000000..2495b2ed1e79 --- /dev/null +++ b/toolchain/common/index_base.h @@ -0,0 +1,48 @@ +// 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 + +#ifndef CARBON_TOOLCHAIN_COMMON_INDEX_BASE_H_ +#define CARBON_TOOLCHAIN_COMMON_INDEX_BASE_H_ + +#include + +#include "common/ostream.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/iterator.h" +#include "llvm/Support/Format.h" + +namespace Carbon { + +template +class DataIterator; + +// A lightweight handle to an item in a vector. +// +// DataIndex is designed to be passed by value, not reference or pointer. They +// are also designed to be small and efficient to store in data structures. +struct IndexBase { + IndexBase() : index(-1) {} + explicit IndexBase(int index) : index(index) {} + + auto Print(llvm::raw_ostream& output) const -> void { output << index; } + + int32_t index; +}; + +template >* = + nullptr> +auto operator==(IndexType lhs, IndexType rhs) -> bool { + return lhs.index == rhs.index; +} +template >* = + nullptr> +auto operator!=(IndexType lhs, IndexType rhs) -> bool { + return lhs.index != rhs.index; +} + +} // namespace Carbon + +#endif // CARBON_TOOLCHAIN_COMMON_INDEX_BASE_H_ diff --git a/toolchain/lexer/BUILD b/toolchain/lexer/BUILD index df3d1535e892..1954862d5550 100644 --- a/toolchain/lexer/BUILD +++ b/toolchain/lexer/BUILD @@ -180,6 +180,7 @@ cc_library( "//common:check", "//common:ostream", "//common:string_helpers", + "//toolchain/common:index_base", "//toolchain/diagnostics:diagnostic_emitter", "//toolchain/source:source_buffer", "@llvm-project//llvm:Support", diff --git a/toolchain/lexer/tokenized_buffer.cpp b/toolchain/lexer/tokenized_buffer.cpp index 582a416e7ba5..a711ea0d6ac6 100644 --- a/toolchain/lexer/tokenized_buffer.cpp +++ b/toolchain/lexer/tokenized_buffer.cpp @@ -733,7 +733,7 @@ auto TokenizedBuffer::IsRecoveryToken(Token token) const -> bool { } auto TokenizedBuffer::GetLineNumber(Line line) const -> int { - return line.index_ + 1; + return line.index + 1; } auto TokenizedBuffer::GetIndentColumnNumber(Line line) const -> int { @@ -742,7 +742,7 @@ auto TokenizedBuffer::GetIndentColumnNumber(Line line) const -> int { auto TokenizedBuffer::GetIdentifierText(Identifier identifier) const -> llvm::StringRef { - return identifier_infos_[identifier.index_].text; + return identifier_infos_[identifier.index].text; } auto TokenizedBuffer::PrintWidths::Widen(const PrintWidths& widths) -> void { @@ -803,7 +803,7 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, Token token, PrintWidths widths) const -> void { widths.Widen(GetTokenPrintWidths(token)); - int token_index = token.index_; + int token_index = token.index; const auto& token_info = GetTokenInfo(token); llvm::StringRef token_text = GetTokenText(token); @@ -825,7 +825,7 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, Token token, switch (token_info.kind) { case TokenKind::Identifier(): - output_stream << ", identifier: " << GetIdentifier(token).index_; + output_stream << ", identifier: " << GetIdentifier(token).index; break; case TokenKind::IntegerLiteral(): output_stream << ", value: `"; @@ -841,10 +841,10 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, Token token, default: if (token_info.kind.IsOpeningSymbol()) { output_stream << ", closing_token: " - << GetMatchedClosingToken(token).index_; + << GetMatchedClosingToken(token).index; } else if (token_info.kind.IsClosingSymbol()) { output_stream << ", opening_token: " - << GetMatchedOpeningToken(token).index_; + << GetMatchedOpeningToken(token).index; } break; } @@ -860,11 +860,11 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, Token token, } auto TokenizedBuffer::GetLineInfo(Line line) -> LineInfo& { - return line_infos_[line.index_]; + return line_infos_[line.index]; } auto TokenizedBuffer::GetLineInfo(Line line) const -> const LineInfo& { - return line_infos_[line.index_]; + return line_infos_[line.index]; } auto TokenizedBuffer::AddLine(LineInfo info) -> Line { @@ -873,11 +873,11 @@ auto TokenizedBuffer::AddLine(LineInfo info) -> Line { } auto TokenizedBuffer::GetTokenInfo(Token token) -> TokenInfo& { - return token_infos_[token.index_]; + return token_infos_[token.index]; } auto TokenizedBuffer::GetTokenInfo(Token token) const -> const TokenInfo& { - return token_infos_[token.index_]; + return token_infos_[token.index]; } auto TokenizedBuffer::AddToken(TokenInfo info) -> Token { @@ -887,7 +887,7 @@ auto TokenizedBuffer::AddToken(TokenInfo info) -> Token { auto TokenizedBuffer::TokenIterator::Print(llvm::raw_ostream& output) const -> void { - output << token_.index_; + output << token_.index; } auto TokenizedBuffer::SourceBufferLocationTranslator::GetLocation( diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index 4b1968517d81..d00d78cfd8a7 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -17,6 +17,7 @@ #include "llvm/ADT/iterator.h" #include "llvm/ADT/iterator_range.h" #include "llvm/Support/raw_ostream.h" +#include "toolchain/common/index_base.h" #include "toolchain/diagnostics/diagnostic_emitter.h" #include "toolchain/lexer/token_kind.h" #include "toolchain/source/source_buffer.h" @@ -43,39 +44,24 @@ namespace Internal { // meaningfully compared. // // All other APIs to query a `Token` are on the `TokenizedBuffer`. -class TokenizedBufferToken { +class TokenizedBufferToken : public IndexBase { public: using Token = TokenizedBufferToken; - TokenizedBufferToken() : TokenizedBufferToken(-1) {} + using IndexBase::IndexBase; - friend auto operator==(Token lhs, Token rhs) -> bool { - return lhs.index_ == rhs.index_; - } - friend auto operator!=(Token lhs, Token rhs) -> bool { - return lhs.index_ != rhs.index_; - } friend auto operator<(Token lhs, Token rhs) -> bool { - return lhs.index_ < rhs.index_; + return lhs.index < rhs.index; } friend auto operator<=(Token lhs, Token rhs) -> bool { - return lhs.index_ <= rhs.index_; + return lhs.index <= rhs.index; } friend auto operator>(Token lhs, Token rhs) -> bool { - return lhs.index_ > rhs.index_; + return lhs.index > rhs.index; } friend auto operator>=(Token lhs, Token rhs) -> bool { - return lhs.index_ >= rhs.index_; + return lhs.index >= rhs.index; } - - auto Print(llvm::raw_ostream& output) const -> void { output << index_; } - - private: - friend TokenizedBuffer; - - explicit TokenizedBufferToken(int index) : index_(index) {} - - int32_t index_; }; } // namespace Internal @@ -104,35 +90,22 @@ class TokenizedBuffer { // same line or the relative position of different lines within the source. // // All other APIs to query a `Line` are on the `TokenizedBuffer`. - class Line { + class Line : public IndexBase { public: - Line() = default; + using IndexBase::IndexBase; - friend auto operator==(Line lhs, Line rhs) -> bool { - return lhs.index_ == rhs.index_; - } - friend auto operator!=(Line lhs, Line rhs) -> bool { - return lhs.index_ != rhs.index_; - } friend auto operator<(Line lhs, Line rhs) -> bool { - return lhs.index_ < rhs.index_; + return lhs.index < rhs.index; } friend auto operator<=(Line lhs, Line rhs) -> bool { - return lhs.index_ <= rhs.index_; + return lhs.index <= rhs.index; } friend auto operator>(Line lhs, Line rhs) -> bool { - return lhs.index_ > rhs.index_; + return lhs.index > rhs.index; } friend auto operator>=(Line lhs, Line rhs) -> bool { - return lhs.index_ >= rhs.index_; + return lhs.index >= rhs.index; } - - private: - friend class TokenizedBuffer; - - explicit Line(int index) : index_(index) {} - - int32_t index_; }; // A lightweight handle to a lexed identifier in a `TokenizedBuffer`. @@ -146,25 +119,8 @@ class TokenizedBuffer { // identifier spelling. Where the identifier was written is not preserved. // // All other APIs to query a `Identifier` are on the `TokenizedBuffer`. - class Identifier { - public: - Identifier() = default; - - // Most normal APIs are provided by the `TokenizedBuffer`, we just support - // basic comparison operations. - friend auto operator==(Identifier lhs, Identifier rhs) -> bool { - return lhs.index_ == rhs.index_; - } - friend auto operator!=(Identifier lhs, Identifier rhs) -> bool { - return lhs.index_ != rhs.index_; - } - - private: - friend class TokenizedBuffer; - - explicit Identifier(int index) : index_(index) {} - - int32_t index_; + class Identifier : public IndexBase { + using IndexBase::IndexBase; }; // Random-access iterator over tokens within the buffer. @@ -187,15 +143,15 @@ class TokenizedBuffer { using iterator_facade_base::operator-; auto operator-(const TokenIterator& rhs) const -> int { - return token_.index_ - rhs.token_.index_; + return token_.index - rhs.token_.index; } auto operator+=(int n) -> TokenIterator& { - token_.index_ += n; + token_.index += n; return *this; } auto operator-=(int n) -> TokenIterator& { - token_.index_ -= n; + token_.index -= n; return *this; }