[lldb] Split ValueObject::CreateChildAtIndex into two functions #94455
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The the function is doing two fairly different things, depending on how it is called. While this allows for some code reuse, it also makes it hard to override it correctly. Possibly for this reason ValueObjectSynthetic overerides GetChildAtIndex instead, which forces it to reimplement some of its functionality, most notably caching of generated children. Splitting this up makes it easier to move the caching to a common place (and hopefully makes the code easier to follow in general).
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe the function is doing two fairly different things, depending on how it is called. While this allows for some code reuse, it also makes it hard to override it correctly. Possibly for this reason ValueObjectSynthetic overerides GetChildAtIndex instead, which forces it to reimplement some of its functionality, most notably caching of generated children. Splitting this up makes it easier to move the caching to a common place (and hopefully makes the code easier to follow in general). Patch is 25.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94455.diff 14 Files Affected:
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
index e7e35e2b2bffc..1e8c7c9c00536 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -959,9 +959,12 @@ class ValueObject {
/// Should only be called by ValueObject::GetChildAtIndex().
///
/// \return A ValueObject managed by this ValueObject's manager.
- virtual ValueObject *CreateChildAtIndex(size_t idx,
- bool synthetic_array_member,
- int32_t synthetic_index);
+ virtual ValueObject *CreateChildAtIndex(size_t idx);
+
+ /// Should only be called by ValueObject::GetSyntheticArrayMember().
+ ///
+ /// \return A ValueObject managed by this ValueObject's manager.
+ virtual ValueObject *CreateSyntheticArrayMember(size_t idx);
/// Should only be called by ValueObject::GetNumChildren().
virtual llvm::Expected<uint32_t>
diff --git a/lldb/include/lldb/Core/ValueObjectConstResult.h b/lldb/include/lldb/Core/ValueObjectConstResult.h
index 37dc0867f26c9..d3b3362bd0e9e 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResult.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResult.h
@@ -79,9 +79,6 @@ class ValueObjectConstResult : public ValueObject {
lldb::ValueObjectSP Dereference(Status &error) override;
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index) override;
-
lldb::ValueObjectSP GetSyntheticChildAtOffset(
uint32_t offset, const CompilerType &type, bool can_create,
ConstString name_const_str = ConstString()) override;
@@ -151,6 +148,13 @@ class ValueObjectConstResult : public ValueObject {
ValueObjectConstResult(ExecutionContextScope *exe_scope,
ValueObjectManager &manager, const Status &error);
+ ValueObject *CreateChildAtIndex(size_t idx) override {
+ return m_impl.CreateChildAtIndex(idx);
+ }
+ ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+ return m_impl.CreateSyntheticArrayMember(idx);
+ }
+
ValueObjectConstResult(const ValueObjectConstResult &) = delete;
const ValueObjectConstResult &
operator=(const ValueObjectConstResult &) = delete;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultCast.h b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
index efcbe0dc6a0bd..911a08363b393 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultCast.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
@@ -35,9 +35,6 @@ class ValueObjectConstResultCast : public ValueObjectCast {
lldb::ValueObjectSP Dereference(Status &error) override;
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index) override;
-
virtual CompilerType GetCompilerType() {
return ValueObjectCast::GetCompilerType();
}
@@ -61,6 +58,13 @@ class ValueObjectConstResultCast : public ValueObjectCast {
friend class ValueObjectConstResult;
friend class ValueObjectConstResultImpl;
+ ValueObject *CreateChildAtIndex(size_t idx) override {
+ return m_impl.CreateChildAtIndex(idx);
+ }
+ ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+ return m_impl.CreateSyntheticArrayMember(idx);
+ }
+
ValueObjectConstResultCast(const ValueObjectConstResultCast &) = delete;
const ValueObjectConstResultCast &
operator=(const ValueObjectConstResultCast &) = delete;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultChild.h b/lldb/include/lldb/Core/ValueObjectConstResultChild.h
index 7e9da14e8e97f..71a3c53befe78 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultChild.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultChild.h
@@ -41,9 +41,6 @@ class ValueObjectConstResultChild : public ValueObjectChild {
lldb::ValueObjectSP Dereference(Status &error) override;
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index) override;
-
virtual CompilerType GetCompilerType() {
return ValueObjectChild::GetCompilerType();
}
@@ -70,6 +67,13 @@ class ValueObjectConstResultChild : public ValueObjectChild {
friend class ValueObjectConstResult;
friend class ValueObjectConstResultImpl;
+ ValueObject *CreateChildAtIndex(size_t idx) override {
+ return m_impl.CreateChildAtIndex(idx);
+ }
+ ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+ return m_impl.CreateSyntheticArrayMember(idx);
+ }
+
ValueObjectConstResultChild(const ValueObjectConstResultChild &) = delete;
const ValueObjectConstResultChild &
operator=(const ValueObjectConstResultChild &) = delete;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultImpl.h b/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
index 5a7a079d3095c..68ba8ae7fba20 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
@@ -38,8 +38,8 @@ class ValueObjectConstResultImpl {
lldb::ValueObjectSP Dereference(Status &error);
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index);
+ ValueObject *CreateChildAtIndex(size_t idx);
+ ValueObject *CreateSyntheticArrayMember(size_t idx);
lldb::ValueObjectSP
GetSyntheticChildAtOffset(uint32_t offset, const CompilerType &type,
diff --git a/lldb/include/lldb/Core/ValueObjectRegister.h b/lldb/include/lldb/Core/ValueObjectRegister.h
index fec8566ba33d9..d948c663a4f8b 100644
--- a/lldb/include/lldb/Core/ValueObjectRegister.h
+++ b/lldb/include/lldb/Core/ValueObjectRegister.h
@@ -49,9 +49,6 @@ class ValueObjectRegisterSet : public ValueObject {
llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index) override;
-
lldb::ValueObjectSP GetChildMemberWithName(llvm::StringRef name,
bool can_create = true) override;
@@ -73,6 +70,11 @@ class ValueObjectRegisterSet : public ValueObject {
ValueObjectManager &manager,
lldb::RegisterContextSP ®_ctx_sp, uint32_t set_idx);
+ ValueObject *CreateChildAtIndex(size_t idx) override;
+ ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+ return nullptr;
+ }
+
// For ValueObject only
ValueObjectRegisterSet(const ValueObjectRegisterSet &) = delete;
const ValueObjectRegisterSet &
diff --git a/lldb/include/lldb/Core/ValueObjectVTable.h b/lldb/include/lldb/Core/ValueObjectVTable.h
index 4662f395a4dde..7087dcc1d1bec 100644
--- a/lldb/include/lldb/Core/ValueObjectVTable.h
+++ b/lldb/include/lldb/Core/ValueObjectVTable.h
@@ -66,9 +66,6 @@ class ValueObjectVTable : public ValueObject {
llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
- ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
- int32_t synthetic_index) override;
-
lldb::ValueType GetValueType() const override;
ConstString GetTypeName() override;
@@ -95,6 +92,11 @@ class ValueObjectVTable : public ValueObject {
private:
ValueObjectVTable(ValueObject &parent);
+ ValueObject *CreateChildAtIndex(size_t idx) override;
+ ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+ return nullptr;
+ }
+
// For ValueObject only
ValueObjectVTable(const ValueObjectVTable &) = delete;
const ValueObjectVTable &operator=(const ValueObjectVTable &) = delete;
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 1443d9dfc3280..0e632f4cf49a3 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -382,7 +382,7 @@ ValueObjectSP ValueObject::GetChildAtIndex(uint32_t idx, bool can_create) {
if (can_create && !m_children.HasChildAtIndex(idx)) {
// No we haven't created the child at this index, so lets have our
// subclass do it and cache the result for quick future access.
- m_children.SetChildAtIndex(idx, CreateChildAtIndex(idx, false, 0));
+ m_children.SetChildAtIndex(idx, CreateChildAtIndex(idx));
}
ValueObject *child = m_children.GetChildAtIndex(idx);
@@ -488,14 +488,10 @@ void ValueObject::SetNumChildren(uint32_t num_children) {
m_children.SetChildrenCount(num_children);
}
-ValueObject *ValueObject::CreateChildAtIndex(size_t idx,
- bool synthetic_array_member,
- int32_t synthetic_index) {
- ValueObject *valobj = nullptr;
-
+ValueObject *ValueObject::CreateChildAtIndex(size_t idx) {
bool omit_empty_base_classes = true;
- bool ignore_array_bounds = synthetic_array_member;
- std::string child_name_str;
+ bool ignore_array_bounds = false;
+ std::string child_name;
uint32_t child_byte_size = 0;
int32_t child_byte_offset = 0;
uint32_t child_bitfield_bit_size = 0;
@@ -503,51 +499,74 @@ ValueObject *ValueObject::CreateChildAtIndex(size_t idx,
bool child_is_base_class = false;
bool child_is_deref_of_parent = false;
uint64_t language_flags = 0;
-
- const bool transparent_pointers = !synthetic_array_member;
+ const bool transparent_pointers = true;
ExecutionContext exe_ctx(GetExecutionContextRef());
auto child_compiler_type_or_err =
GetCompilerType().GetChildCompilerTypeAtIndex(
&exe_ctx, idx, transparent_pointers, omit_empty_base_classes,
- ignore_array_bounds, child_name_str, child_byte_size,
- child_byte_offset, child_bitfield_bit_size, child_bitfield_bit_offset,
+ ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
+ child_bitfield_bit_size, child_bitfield_bit_offset,
child_is_base_class, child_is_deref_of_parent, this, language_flags);
- CompilerType child_compiler_type;
- if (!child_compiler_type_or_err)
+ if (!child_compiler_type_or_err || !child_compiler_type_or_err->IsValid()) {
LLDB_LOG_ERROR(GetLog(LLDBLog::Types),
child_compiler_type_or_err.takeError(),
"could not find child: {0}");
- else
- child_compiler_type = *child_compiler_type_or_err;
+ return nullptr;
+ }
+
+ return new ValueObjectChild(
+ *this, *child_compiler_type_or_err, ConstString(child_name),
+ child_byte_size, child_byte_offset, child_bitfield_bit_size,
+ child_bitfield_bit_offset, child_is_base_class, child_is_deref_of_parent,
+ eAddressTypeInvalid, language_flags);
+}
+
+ValueObject *ValueObject::CreateSyntheticArrayMember(size_t idx) {
+ bool omit_empty_base_classes = true;
+ bool ignore_array_bounds = true;
+ std::string child_name;
+ uint32_t child_byte_size = 0;
+ int32_t child_byte_offset = 0;
+ uint32_t child_bitfield_bit_size = 0;
+ uint32_t child_bitfield_bit_offset = 0;
+ bool child_is_base_class = false;
+ bool child_is_deref_of_parent = false;
+ uint64_t language_flags = 0;
+ const bool transparent_pointers = false;
- if (child_compiler_type) {
- if (synthetic_index)
- child_byte_offset += child_byte_size * synthetic_index;
+ ExecutionContext exe_ctx(GetExecutionContextRef());
- ConstString child_name;
- if (!child_name_str.empty())
- child_name.SetCString(child_name_str.c_str());
+ auto child_compiler_type_or_err =
+ GetCompilerType().GetChildCompilerTypeAtIndex(
+ &exe_ctx, 0, transparent_pointers, omit_empty_base_classes,
+ ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
+ child_bitfield_bit_size, child_bitfield_bit_offset,
+ child_is_base_class, child_is_deref_of_parent, this, language_flags);
+ if (!child_compiler_type_or_err) {
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Types),
+ child_compiler_type_or_err.takeError(),
+ "could not find child: {0}");
+ return nullptr;
+ }
+
+ if (child_compiler_type_or_err->IsValid()) {
+ child_byte_offset += child_byte_size * idx;
- valobj = new ValueObjectChild(
- *this, child_compiler_type, child_name, child_byte_size,
- child_byte_offset, child_bitfield_bit_size, child_bitfield_bit_offset,
- child_is_base_class, child_is_deref_of_parent, eAddressTypeInvalid,
- language_flags);
+ return new ValueObjectChild(
+ *this, *child_compiler_type_or_err, ConstString(child_name),
+ child_byte_size, child_byte_offset, child_bitfield_bit_size,
+ child_bitfield_bit_offset, child_is_base_class,
+ child_is_deref_of_parent, eAddressTypeInvalid, language_flags);
}
// In case of an incomplete type, try to use the ValueObject's
// synthetic value to create the child ValueObject.
- if (!valobj && synthetic_array_member) {
- if (ValueObjectSP synth_valobj_sp = GetSyntheticValue()) {
- valobj = synth_valobj_sp
- ->GetChildAtIndex(synthetic_index, synthetic_array_member)
- .get();
- }
- }
+ if (ValueObjectSP synth_valobj_sp = GetSyntheticValue())
+ return synth_valobj_sp->GetChildAtIndex(idx, /*can_create=*/true).get();
- return valobj;
+ return nullptr;
}
bool ValueObject::GetSummaryAsCString(TypeSummaryImpl *summary_ptr,
@@ -1616,7 +1635,7 @@ ValueObjectSP ValueObject::GetSyntheticArrayMember(size_t index,
ValueObject *synthetic_child;
// We haven't made a synthetic array member for INDEX yet, so lets make
// one and cache it for any future reference.
- synthetic_child = CreateChildAtIndex(0, true, index);
+ synthetic_child = CreateSyntheticArrayMember(index);
// Cache the value if we got one back...
if (synthetic_child) {
diff --git a/lldb/source/Core/ValueObjectConstResult.cpp b/lldb/source/Core/ValueObjectConstResult.cpp
index 8ac2c1cac2f66..879d3c3f6b037 100644
--- a/lldb/source/Core/ValueObjectConstResult.cpp
+++ b/lldb/source/Core/ValueObjectConstResult.cpp
@@ -267,12 +267,6 @@ lldb::addr_t ValueObjectConstResult::GetAddressOf(bool scalar_is_load_address,
return m_impl.GetAddressOf(scalar_is_load_address, address_type);
}
-ValueObject *ValueObjectConstResult::CreateChildAtIndex(
- size_t idx, bool synthetic_array_member, int32_t synthetic_index) {
- return m_impl.CreateChildAtIndex(idx, synthetic_array_member,
- synthetic_index);
-}
-
size_t ValueObjectConstResult::GetPointeeData(DataExtractor &data,
uint32_t item_idx,
uint32_t item_count) {
diff --git a/lldb/source/Core/ValueObjectConstResultCast.cpp b/lldb/source/Core/ValueObjectConstResultCast.cpp
index fceb2635f876f..bf7a12dc68236 100644
--- a/lldb/source/Core/ValueObjectConstResultCast.cpp
+++ b/lldb/source/Core/ValueObjectConstResultCast.cpp
@@ -44,12 +44,6 @@ lldb::ValueObjectSP ValueObjectConstResultCast::AddressOf(Status &error) {
return m_impl.AddressOf(error);
}
-ValueObject *ValueObjectConstResultCast::CreateChildAtIndex(
- size_t idx, bool synthetic_array_member, int32_t synthetic_index) {
- return m_impl.CreateChildAtIndex(idx, synthetic_array_member,
- synthetic_index);
-}
-
size_t ValueObjectConstResultCast::GetPointeeData(DataExtractor &data,
uint32_t item_idx,
uint32_t item_count) {
diff --git a/lldb/source/Core/ValueObjectConstResultChild.cpp b/lldb/source/Core/ValueObjectConstResultChild.cpp
index 36bf11a0b73af..39fc0c9fbb35b 100644
--- a/lldb/source/Core/ValueObjectConstResultChild.cpp
+++ b/lldb/source/Core/ValueObjectConstResultChild.cpp
@@ -56,12 +56,6 @@ lldb::addr_t ValueObjectConstResultChild::GetAddressOf(
return m_impl.GetAddressOf(scalar_is_load_address, address_type);
}
-ValueObject *ValueObjectConstResultChild::CreateChildAtIndex(
- size_t idx, bool synthetic_array_member, int32_t synthetic_index) {
- return m_impl.CreateChildAtIndex(idx, synthetic_array_member,
- synthetic_index);
-}
-
size_t ValueObjectConstResultChild::GetPointeeData(DataExtractor &data,
uint32_t item_idx,
uint32_t item_count) {
diff --git a/lldb/source/Core/ValueObjectConstResultImpl.cpp b/lldb/source/Core/ValueObjectConstResultImpl.cpp
index 493980d7ea960..2a7c907700765 100644
--- a/lldb/source/Core/ValueObjectConstResultImpl.cpp
+++ b/lldb/source/Core/ValueObjectConstResultImpl.cpp
@@ -46,18 +46,15 @@ lldb::ValueObjectSP ValueObjectConstResultImpl::Dereference(Status &error) {
return m_impl_backend->ValueObject::Dereference(error);
}
-ValueObject *ValueObjectConstResultImpl::CreateChildAtIndex(
- size_t idx, bool synthetic_array_member, int32_t synthetic_index) {
+ValueObject *ValueObjectConstResultImpl::CreateChildAtIndex(size_t idx) {
if (m_impl_backend == nullptr)
return nullptr;
m_impl_backend->UpdateValueIfNeeded(false);
- ValueObjectConstResultChild *valobj = nullptr;
-
bool omit_empty_base_classes = true;
- bool ignore_array_bounds = synthetic_array_member;
- std::string child_name_str;
+ bool ignore_array_bounds = false;
+ std::string child_name;
uint32_t child_byte_size = 0;
int32_t child_byte_offset = 0;
uint32_t child_bitfield_bit_size = 0;
@@ -65,56 +62,100 @@ ValueObject *ValueObjectConstResultImpl::CreateChildAtIndex(
bool child_is_base_class = false;
bool child_is_deref_of_parent = false;
uint64_t language_flags;
-
- const bool transparent_pointers = !synthetic_array_member;
+ const bool transparent_pointers = true;
CompilerType compiler_type = m_impl_backend->GetCompilerType();
ExecutionContext exe_ctx(m_impl_backend->GetExecutionContextRef());
auto child_compiler_type_or_err = compiler_type.GetChildCompilerTypeAtIndex(
&exe_ctx, idx, transparent_pointers, omit_empty_base_classes,
- ignore_array_bounds, child_name_str, child_byte_size, child_byte_offset,
+ ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
child_bitfield_bit_size, child_bitfield_bit_offset, child_is_base_class,
child_is_deref_of_parent, m_impl_backend, language_flags);
- CompilerType child_compiler_type;
- if (!child_compiler_type_or_err)
+
+ // One might think we should check that the size of the children
+ // is always strictly positive, hence we could avoid creating a
+ // ValueObject if that's not the case, but it turns out there
+ // are languages out there which allow zero-size types with
+ // children (e.g. Swift).
+ if (!child_compiler_type_or_err || !child_compiler_type_or_err->IsValid()) {
LLDB_LOG_ERROR(GetLog(LLDBLog::Types),
child_compiler_type_or_err.takeError(),
"could not find child: {0}");
- else
- child_compiler_type = *child_compiler_type_or_err;
+ return nullptr;
+ }
+ lldb::addr_t child_live_addr = LLDB_INVALID_ADDRESS;
+ // Transfer the live address (with offset) to the child. But if
+ // the parent is a pointer, the live address is where that pointer
+ // value lives in memory, so the children live addresses aren't
+ // offsets from that value, they are just other load addresses that
+ // are recorded in the Value of the child ValueObjects.
+ if (m_live_address != LLDB_INVALID_ADDRESS && !compiler_type.IsPointerType())
+ child_live_addr = m_live_address + child_byte_offset;
+
+ return new ValueObjectConstResultChild(
+ *m_impl_backend, *child_compiler_type_or_err, ConstString(child_name),
+ child_byte_size, child_byte_offset, child_bitfield_bit_size,
+ child_bitfield_bit_offset, child_is_base_class, child_is_deref_of_parent,
+ child_live_addr, language_flags);
+}
+
+ValueObject *
+ValueObjectConstResultImpl::CreateSyntheticArrayMember(size_t idx) {
+ if (m_impl_backend == nullptr)
+ return nullptr;
+
+ m_impl_backend->UpdateValueIfNeeded(false);
+
+ bool omit_empty_base_classes = true;
+ bool ignore_array_bounds = true;
+ std::string child_name;
+ uint32_t child_byte_size = 0;
+ int32_t child_byte_offset = 0;
+ uint32_t child_bitfield_bit_size = 0;
+ ...
[truncated]
|
labath
added a commit
to labath/gala
that referenced
this pull request
Jun 5, 2024
My llvm/llvm-project#94455 broke getitem_with_gdbvalue_index test (it changed the name of anonymous unions from None to ""). This PR fixes the implementation to use a more reliable way of detecting anonymous unions. It also fixes two other issues: - it now looks into anonymous structs as well as unions - it recurses into anonymous types Last, I've refactored the associated test to remove the duplication of expectations and better expose the difference between the two tested scenarios.
labath
commented
Jun 5, 2024
Comment on lines
-529
to
-531
| ConstString child_name; | ||
| if (!child_name_str.empty()) | ||
| child_name.SetCString(child_name_str.c_str()); |
There was a problem hiding this comment.
It seems that one side effect of "simplifying" this is that the name of anonymous struct members changes from nullptr/None to "". None of lldb tests broke but this broke some (rather dodgy) downstream code of ours. I would be inclined to keep this change, as I don't think we can make a promise (and keep it) to preserve details like this.
slackito
pushed a commit
to sivachandra/gala
that referenced
this pull request
Jun 5, 2024
My llvm/llvm-project#94455 broke getitem_with_gdbvalue_index test (it changed the name of anonymous unions from None to ""). This PR fixes the implementation to use a more reliable way of detecting anonymous unions. It also fixes two other issues: - it now looks into anonymous structs as well as unions - it recurses into anonymous types Last, I've refactored the associated test to remove the duplication of expectations and better expose the difference between the two tested scenarios.
jimingham
reviewed
Jun 6, 2024
| &exe_ctx, 0, transparent_pointers, omit_empty_base_classes, | ||
| ignore_array_bounds, child_name, child_byte_size, child_byte_offset, | ||
| child_bitfield_bit_size, child_bitfield_bit_offset, child_is_base_class, | ||
| child_is_deref_of_parent, m_impl_backend, language_flags); | ||
| // One might think we should check that the size of the children |
There was a problem hiding this comment.
Shouldn't this version of this comment be removed? It appears at line 76 in your changed version.
There was a problem hiding this comment.
Ah, no, didn't read far enough. You do need to repeat the comment.
jimingham
approved these changes
Jun 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The the function is doing two fairly different things, depending on how it is called. While this allows for some code reuse, it also makes it hard to override it correctly. Possibly for this reason ValueObjectSynthetic overerides GetChildAtIndex instead, which forces it to reimplement some of its functionality, most notably caching of generated children.
Splitting this up makes it easier to move the caching to a common place (and hopefully makes the code easier to follow in general).