From 6e2dbb5b6165e68ca16b005836ee656d09b6e432 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Tue, 1 Apr 2025 16:40:28 +0200 Subject: [PATCH] Change `CopyOnWriteBlock::file_` from reference to pointer (#5230) Per [the style guide](https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md#syntax-and-formatting): * If it is captured and must outlive the call expression itself, use a pointer and document that it must not be null (unless it is also optional). * When storing an object's address as a non-owned member, prefer storing a pointer. --- toolchain/check/convert.cpp | 12 ++++++------ toolchain/check/subst.cpp | 6 +++--- toolchain/sem_ir/copy_on_write_block.h | 20 ++++++++++---------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 234b1eac7d1c..5cd57eb99932 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -385,10 +385,10 @@ static auto ConvertTupleToTuple(Context& context, SemIR::TupleType src_type, // TODO: Annotate diagnostics coming from here with the element index. auto new_block = literal_elems_id.has_value() - ? SemIR::CopyOnWriteInstBlock(sem_ir, literal_elems_id) + ? SemIR::CopyOnWriteInstBlock(&sem_ir, literal_elems_id) : SemIR::CopyOnWriteInstBlock( - sem_ir, SemIR::CopyOnWriteInstBlock::UninitializedBlock{ - src_elem_types.size()}); + &sem_ir, SemIR::CopyOnWriteInstBlock::UninitializedBlock{ + src_elem_types.size()}); for (auto [i, src_type_id, dest_type_id] : llvm::enumerate(src_elem_types, dest_elem_types)) { // TODO: This call recurses back into conversion. Switch to an iterative @@ -496,10 +496,10 @@ static auto ConvertStructToStructOrClass(Context& context, // TODO: Annotate diagnostics coming from here with the element index. auto new_block = literal_elems_id.has_value() && !dest_has_vptr - ? SemIR::CopyOnWriteInstBlock(sem_ir, literal_elems_id) + ? SemIR::CopyOnWriteInstBlock(&sem_ir, literal_elems_id) : SemIR::CopyOnWriteInstBlock( - sem_ir, SemIR::CopyOnWriteInstBlock::UninitializedBlock{ - dest_elem_fields.size()}); + &sem_ir, SemIR::CopyOnWriteInstBlock::UninitializedBlock{ + dest_elem_fields.size()}); for (auto [i, dest_field] : llvm::enumerate(dest_elem_fields)) { if (dest_field.name_id == SemIR::NameId::Vptr) { if constexpr (!ToClass) { diff --git a/toolchain/check/subst.cpp b/toolchain/check/subst.cpp index acb190c232b8..0b3d8c9c2cf9 100644 --- a/toolchain/check/subst.cpp +++ b/toolchain/check/subst.cpp @@ -155,7 +155,7 @@ static auto PopOperand(Context& context, Worklist& worklist, SemIR::Inst::ArgAndKind arg) -> int32_t { auto pop_block_id = [&](SemIR::InstBlockId old_inst_block_id) { auto size = context.inst_blocks().Get(old_inst_block_id).size(); - SemIR::CopyOnWriteInstBlock new_inst_block(context.sem_ir(), + SemIR::CopyOnWriteInstBlock new_inst_block(&context.sem_ir(), old_inst_block_id); for (auto i : llvm::reverse(llvm::seq(size))) { new_inst_block.Set(i, worklist.Pop()); @@ -196,7 +196,7 @@ static auto PopOperand(Context& context, Worklist& worklist, } case CARBON_KIND(SemIR::StructTypeFieldsId old_fields_id): { auto old_fields = context.struct_type_fields().Get(old_fields_id); - SemIR::CopyOnWriteStructTypeFieldsBlock new_fields(context.sem_ir(), + SemIR::CopyOnWriteStructTypeFieldsBlock new_fields(&context.sem_ir(), old_fields_id); for (auto i : llvm::reverse(llvm::seq(old_fields.size()))) { new_fields.Set(i, {.name_id = old_fields[i].name_id, @@ -207,7 +207,7 @@ static auto PopOperand(Context& context, Worklist& worklist, } case CARBON_KIND(SemIR::TypeBlockId old_type_block_id): { auto size = context.type_blocks().Get(old_type_block_id).size(); - SemIR::CopyOnWriteTypeBlock new_type_block(context.sem_ir(), + SemIR::CopyOnWriteTypeBlock new_type_block(&context.sem_ir(), old_type_block_id); for (auto i : llvm::reverse(llvm::seq(size))) { new_type_block.Set( diff --git a/toolchain/sem_ir/copy_on_write_block.h b/toolchain/sem_ir/copy_on_write_block.h index dc69ad97ec0d..2c6c4a78986d 100644 --- a/toolchain/sem_ir/copy_on_write_block.h +++ b/toolchain/sem_ir/copy_on_write_block.h @@ -27,16 +27,16 @@ class CopyOnWriteBlock { }; // Constructs the block. `source_id` is used as the initial value of the - // block. - explicit CopyOnWriteBlock(SemIR::File& file, BlockIdType source_id) + // block. `file` must not be null. + explicit CopyOnWriteBlock(SemIR::File* file, BlockIdType source_id) : file_(file), source_id_(source_id) {} // Constructs the block, treating the original block as an uninitialized block - // with `size` elements. - explicit CopyOnWriteBlock(SemIR::File& file, UninitializedBlock uninit) + // with `size` elements. `file` must not be null. + explicit CopyOnWriteBlock(SemIR::File* file, UninitializedBlock uninit) : file_(file), source_id_(BlockIdType::None), - id_((file_.*ValueStore)().AddUninitialized(uninit.size)) {} + id_((file_->*ValueStore)().AddUninitialized(uninit.size)) {} // Gets a block ID containing the resulting elements. Note that further // modifications may or may not allocate a new ID, so this should only be @@ -46,23 +46,23 @@ class CopyOnWriteBlock { // Gets a canonical block ID containing the resulting elements. This assumes // the original block ID, if specified, was also canonical. auto GetCanonical() const -> BlockIdType { - return id_ == source_id_ ? id_ : (file_.*ValueStore)().MakeCanonical(id_); + return id_ == source_id_ ? id_ : (file_->*ValueStore)().MakeCanonical(id_); } // Sets the element at index `i` within the block. Lazily allocates a new // block when the value changes for the first time. auto Set(int i, typename BlockIdType::ElementType value) -> void { - if (source_id_.has_value() && (file_.*ValueStore)().Get(id_)[i] == value) { + if (source_id_.has_value() && (file_->*ValueStore)().Get(id_)[i] == value) { return; } if (id_ == source_id_) { - id_ = (file_.*ValueStore)().Add((file_.*ValueStore)().Get(source_id_)); + id_ = (file_->*ValueStore)().Add((file_->*ValueStore)().Get(source_id_)); } - (file_.*ValueStore)().GetMutable(id_)[i] = value; + (file_->*ValueStore)().GetMutable(id_)[i] = value; } private: - SemIR::File& file_; + SemIR::File* file_; BlockIdType source_id_; BlockIdType id_ = source_id_; };