[lldb][TypeSynthetic][NFC] Make SyntheticChildrenFrontend::Update() return an enum #80167
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
Michael137
requested review from
kastiglione,
adrian-prantl and
felipepiovezan
Michael137
changed the title
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch changes the return value of Patch is 71.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80167.diff 33 Files Affected:
diff --git a/lldb/include/lldb/DataFormatters/TypeSynthetic.h b/lldb/include/lldb/DataFormatters/TypeSynthetic.h
index 41be9b7efda8f..407177bbd303e 100644
--- a/lldb/include/lldb/DataFormatters/TypeSynthetic.h
+++ b/lldb/include/lldb/DataFormatters/TypeSynthetic.h
@@ -49,14 +49,17 @@ class SyntheticChildrenFrontEnd {
virtual size_t GetIndexOfChildWithName(ConstString name) = 0;
- // this function is assumed to always succeed and it if fails, the front-end
- // should know to deal with it in the correct way (most probably, by refusing
- // to return any children) the return value of Update() should actually be
- // interpreted as "ValueObjectSyntheticFilter cache is good/bad" if =true,
- // ValueObjectSyntheticFilter is allowed to use the children it fetched
- // previously and cached if =false, ValueObjectSyntheticFilter must throw
- // away its cache, and query again for children
- virtual bool Update() = 0;
+ enum class CacheState : short { Invalid, Valid };
+
+ /// This function is assumed to always succeed and if it fails, the front-end
+ /// should know to deal with it in the correct way (most probably, by refusing
+ /// to return any children). The return value of \ref Update should actually
+ /// be interpreted as "ValueObjectSyntheticFilter cache is good/bad". If this
+ /// function returns \ref CacheState::Valid, \ref ValueObjectSyntheticFilter
+ /// is allowed to use the children it fetched previously and cached.
+ /// Otherwise, \ref ValueObjectSyntheticFilter must throw away its cache, and
+ /// query again for children.
+ virtual CacheState Update() = 0;
// if this function returns false, then CalculateNumChildren() MUST return 0
// since UI frontends might validly decide not to inquire for children given
@@ -116,7 +119,7 @@ class SyntheticValueProviderFrontEnd : public SyntheticChildrenFrontEnd {
return UINT32_MAX;
}
- bool Update() override { return false; }
+ CacheState Update() override { return CacheState::Invalid; }
bool MightHaveChildren() override { return false; }
@@ -328,7 +331,7 @@ class TypeFilterImpl : public SyntheticChildren {
filter->GetExpressionPathAtIndex(idx), true);
}
- bool Update() override { return false; }
+ CacheState Update() override { return CacheState::Invalid; }
bool MightHaveChildren() override { return filter->GetCount() > 0; }
@@ -427,7 +430,7 @@ class ScriptedSyntheticChildren : public SyntheticChildren {
lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
- bool Update() override;
+ CacheState Update() override;
bool MightHaveChildren() override;
diff --git a/lldb/include/lldb/DataFormatters/VectorIterator.h b/lldb/include/lldb/DataFormatters/VectorIterator.h
index 3414298f255b6..df11380907099 100644
--- a/lldb/include/lldb/DataFormatters/VectorIterator.h
+++ b/lldb/include/lldb/DataFormatters/VectorIterator.h
@@ -28,7 +28,7 @@ class VectorIteratorSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
- bool Update() override;
+ CacheState Update() override;
bool MightHaveChildren() override;
diff --git a/lldb/source/Core/ValueObjectSyntheticFilter.cpp b/lldb/source/Core/ValueObjectSyntheticFilter.cpp
index 43bc532c4a041..33f68615059bf 100644
--- a/lldb/source/Core/ValueObjectSyntheticFilter.cpp
+++ b/lldb/source/Core/ValueObjectSyntheticFilter.cpp
@@ -43,7 +43,7 @@ class DummySyntheticFrontEnd : public SyntheticChildrenFrontEnd {
bool MightHaveChildren() override { return m_backend.MightHaveChildren(); }
- bool Update() override { return false; }
+ CacheState Update() override { return CacheState::Invalid; }
};
ValueObjectSynthetic::ValueObjectSynthetic(ValueObject &parent,
@@ -177,7 +177,8 @@ bool ValueObjectSynthetic::UpdateValue() {
}
// let our backend do its update
- if (!m_synth_filter_up->Update()) {
+ if (m_synth_filter_up->Update() ==
+ SyntheticChildrenFrontEnd::CacheState::Invalid) {
LLDB_LOGF(log,
"[ValueObjectSynthetic::UpdateValue] name=%s, synthetic "
"filter said caches are stale - clearing",
diff --git a/lldb/source/DataFormatters/TypeSynthetic.cpp b/lldb/source/DataFormatters/TypeSynthetic.cpp
index de042e474903e..10174c034a0ea 100644
--- a/lldb/source/DataFormatters/TypeSynthetic.cpp
+++ b/lldb/source/DataFormatters/TypeSynthetic.cpp
@@ -190,11 +190,14 @@ size_t ScriptedSyntheticChildren::FrontEnd::CalculateNumChildren(uint32_t max) {
return m_interpreter->CalculateNumChildren(m_wrapper_sp, max);
}
-bool ScriptedSyntheticChildren::FrontEnd::Update() {
+SyntheticChildrenFrontEnd::CacheState
+ScriptedSyntheticChildren::FrontEnd::Update() {
if (!m_wrapper_sp || m_interpreter == nullptr)
- return false;
+ return CacheState::Invalid;
- return m_interpreter->UpdateSynthProviderInstance(m_wrapper_sp);
+ return m_interpreter->UpdateSynthProviderInstance(m_wrapper_sp)
+ ? CacheState::Valid
+ : CacheState::Invalid;
}
bool ScriptedSyntheticChildren::FrontEnd::MightHaveChildren() {
diff --git a/lldb/source/DataFormatters/VectorType.cpp b/lldb/source/DataFormatters/VectorType.cpp
index 57dae0b2c71f0..fc3900dc6ebce 100644
--- a/lldb/source/DataFormatters/VectorType.cpp
+++ b/lldb/source/DataFormatters/VectorType.cpp
@@ -245,7 +245,7 @@ class VectorTypeSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
return child_sp;
}
- bool Update() override {
+ CacheState Update() override {
m_parent_format = m_backend.GetFormat();
CompilerType parent_type(m_backend.GetCompilerType());
CompilerType element_type;
@@ -258,7 +258,7 @@ class VectorTypeSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
::CalculateNumChildren(element_type, num_elements, m_child_type)
.value_or(0);
m_item_format = GetItemFormatForFormat(m_parent_format, m_child_type);
- return false;
+ return CacheState::Invalid;
}
bool MightHaveChildren() override { return true; }
diff --git a/lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp b/lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp
index 314a4aca8d266..3e9eb55e090c7 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/BlockPointer.cpp
@@ -136,7 +136,7 @@ class BlockPointerSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
// return true if this object is now safe to use forever without ever
// updating again; the typical (and tested) answer here is 'false'
- bool Update() override { return false; }
+ CacheState Update() override { return CacheState::Invalid; }
// maybe return false if the block pointer is, say, null
bool MightHaveChildren() override { return true; }
diff --git a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
index 6aeae97667c16..3a40b62bce3fb 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -125,24 +125,24 @@ lldb::ValueObjectSP lldb_private::formatters::
return lldb::ValueObjectSP();
}
-bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
- Update() {
+SyntheticChildrenFrontEnd::CacheState
+lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::Update() {
m_resume_ptr_sp.reset();
m_destroy_ptr_sp.reset();
m_promise_ptr_sp.reset();
ValueObjectSP valobj_sp = m_backend.GetNonSyntheticValue();
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
lldb::addr_t frame_ptr_addr = GetCoroFramePtrFromHandle(valobj_sp);
if (frame_ptr_addr == 0 || frame_ptr_addr == LLDB_INVALID_ADDRESS)
- return false;
+ return CacheState::Invalid;
auto ts = valobj_sp->GetCompilerType().GetTypeSystem();
auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
if (!ast_ctx)
- return false;
+ return CacheState::Invalid;
// Create the `resume` and `destroy` children.
lldb::TargetSP target_sp = m_backend.GetTargetSP();
@@ -165,7 +165,7 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
CompilerType promise_type(
valobj_sp->GetCompilerType().GetTypeTemplateArgument(0));
if (!promise_type)
- return false;
+ return CacheState::Invalid;
// Try to infer the promise_type if it was type-erased
if (promise_type.IsVoidType()) {
@@ -180,7 +180,7 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
// If we don't know the promise type, we don't display the `promise` member.
// `CreateValueObjectFromAddress` below would fail for `void` types.
if (promise_type.IsVoidType()) {
- return false;
+ return CacheState::Invalid;
}
// Add the `promise` member. We intentionally add `promise` as a pointer type
@@ -194,7 +194,7 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
if (error.Success())
m_promise_ptr_sp = promisePtr->Clone(ConstString("promise"));
- return false;
+ return CacheState::Invalid;
}
bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
diff --git a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
index b26cc9ed6132d..23f16d2a35110 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -38,7 +38,7 @@ class StdlibCoroutineHandleSyntheticFrontEnd
lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
- bool Update() override;
+ CacheState Update() override;
bool MightHaveChildren() override;
diff --git a/lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp b/lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
index 2876efc5c41a5..4d4e4fd85f7ff 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
@@ -33,7 +33,7 @@ class GenericBitsetFrontEnd : public SyntheticChildrenFrontEnd {
}
bool MightHaveChildren() override { return true; }
- bool Update() override;
+ CacheState Update() override;
size_t CalculateNumChildren() override { return m_elements.size(); }
ValueObjectSP GetChildAtIndex(size_t idx) override;
@@ -78,13 +78,13 @@ llvm::StringRef GenericBitsetFrontEnd::GetDataContainerMemberName() {
llvm_unreachable("Unknown StdLib enum");
}
-bool GenericBitsetFrontEnd::Update() {
+SyntheticChildrenFrontEnd::CacheState GenericBitsetFrontEnd::Update() {
m_elements.clear();
m_first = nullptr;
TargetSP target_sp = m_backend.GetTargetSP();
if (!target_sp)
- return false;
+ return CacheState::Invalid;
size_t size = 0;
@@ -94,7 +94,7 @@ bool GenericBitsetFrontEnd::Update() {
m_elements.assign(size, ValueObjectSP());
m_first =
m_backend.GetChildMemberWithName(GetDataContainerMemberName()).get();
- return false;
+ return CacheState::Invalid;
}
ValueObjectSP GenericBitsetFrontEnd::GetChildAtIndex(size_t idx) {
diff --git a/lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp b/lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp
index 7415e915844fc..55b70113b6c7d 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/GenericOptional.cpp
@@ -44,7 +44,7 @@ class GenericOptionalFrontend : public SyntheticChildrenFrontEnd {
size_t CalculateNumChildren() override { return m_has_value ? 1U : 0U; }
ValueObjectSP GetChildAtIndex(size_t idx) override;
- bool Update() override;
+ CacheState Update() override;
private:
bool m_has_value = false;
@@ -61,7 +61,7 @@ GenericOptionalFrontend::GenericOptionalFrontend(ValueObject &valobj,
}
}
-bool GenericOptionalFrontend::Update() {
+SyntheticChildrenFrontEnd::CacheState GenericOptionalFrontend::Update() {
ValueObjectSP engaged_sp;
if (m_stdlib == StdLib::LibCxx)
@@ -71,14 +71,14 @@ bool GenericOptionalFrontend::Update() {
->GetChildMemberWithName("_M_engaged");
if (!engaged_sp)
- return false;
+ return CacheState::Invalid;
// _M_engaged/__engaged is a bool flag and is true if the optional contains a
// value. Converting it to unsigned gives us a size of 1 if it contains a
// value and 0 if not.
m_has_value = engaged_sp->GetValueAsUnsigned(0) != 0;
- return false;
+ return CacheState::Invalid;
}
ValueObjectSP GenericOptionalFrontend::GetChildAtIndex(size_t _idx) {
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index d0bdbe1fd4d91..74941baeb67fa 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -231,21 +231,22 @@ lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
Update();
}
-bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
+SyntheticChildrenFrontEnd::CacheState
+lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
m_pair_sp.reset();
m_pair_ptr = nullptr;
ValueObjectSP valobj_sp = m_backend.GetSP();
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
TargetSP target_sp(valobj_sp->GetTargetSP());
if (!target_sp)
- return false;
+ return CacheState::Invalid;
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
// this must be a ValueObject* because it is a child of the ValueObject we
// are producing children for it if were a ValueObjectSP, we would end up
@@ -278,7 +279,7 @@ bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
auto __i_(valobj_sp->GetChildMemberWithName("__i_"));
if (!__i_) {
m_pair_ptr = nullptr;
- return false;
+ return CacheState::Invalid;
}
CompilerType pair_type(
__i_->GetCompilerType().GetTypeTemplateArgument(0));
@@ -290,7 +291,7 @@ bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
if (!pair_type) {
m_pair_ptr = nullptr;
- return false;
+ return CacheState::Invalid;
}
auto addr(m_pair_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS));
@@ -299,7 +300,7 @@ bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
auto ts = pair_type.GetTypeSystem();
auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
if (!ast_ctx)
- return false;
+ return CacheState::Invalid;
// Mimick layout of std::__tree_iterator::__ptr_ and read it in
// from process memory.
@@ -328,14 +329,14 @@ bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
{"payload", pair_type}});
std::optional<uint64_t> size = tree_node_type.GetByteSize(nullptr);
if (!size)
- return false;
+ return CacheState::Invalid;
WritableDataBufferSP buffer_sp(new DataBufferHeap(*size, 0));
ProcessSP process_sp(target_sp->GetProcessSP());
Status error;
process_sp->ReadMemory(addr, buffer_sp->GetBytes(),
buffer_sp->GetByteSize(), error);
if (error.Fail())
- return false;
+ return CacheState::Invalid;
DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
process_sp->GetAddressByteSize());
auto pair_sp = CreateValueObjectFromData(
@@ -347,7 +348,7 @@ bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
}
}
- return false;
+ return CacheState::Invalid;
}
size_t lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
@@ -399,22 +400,22 @@ lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
Update();
}
-bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
- Update() {
+SyntheticChildrenFrontEnd::CacheState lldb_private::formatters::
+ LibCxxUnorderedMapIteratorSyntheticFrontEnd::Update() {
m_pair_sp.reset();
m_iter_ptr = nullptr;
ValueObjectSP valobj_sp = m_backend.GetSP();
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
TargetSP target_sp(valobj_sp->GetTargetSP());
if (!target_sp)
- return false;
+ return CacheState::Invalid;
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
auto exprPathOptions = ValueObject::GetValueForExpressionPathOptions()
.DontCheckDotVsArrowSyntax()
@@ -437,7 +438,7 @@ bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
auto iter_child(valobj_sp->GetChildMemberWithName("__i_"));
if (!iter_child) {
m_iter_ptr = nullptr;
- return false;
+ return CacheState::Invalid;
}
CompilerType node_type(iter_child->GetCompilerType()
@@ -455,19 +456,19 @@ bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
if (!pair_type) {
m_iter_ptr = nullptr;
- return false;
+ return CacheState::Invalid;
}
uint64_t addr = m_iter_ptr->GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
m_iter_ptr = nullptr;
if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
- return false;
+ return CacheState::Invalid;
auto ts = pair_type.GetTypeSystem();
auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
if (!ast_ctx)
- return false;
+ return CacheState::Invalid;
// Mimick layout of std::__hash_iterator::__node_ and read it in
// from process memory.
@@ -489,14 +490,14 @@ bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
{"__value_", pair_type}});
std::optional<uint64_t> size = tree_node_type.GetByteSize(nullptr);
if (!size)
- return false;
+ return CacheState::Invalid;
WritableDataBufferSP buffer_sp(new DataBufferHeap(*size, 0));
ProcessSP process_sp(target_sp->GetProcessSP());
Status error;
process_sp->ReadMemory(addr, buffer_sp->GetBytes(),
buffer_sp->GetByteSize(), error);
if (error.Fail())
- return false;
+ return CacheState::Invalid;
DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
process_sp->GetAddressByteSize());
auto pair_sp = CreateValueObjectFromData(
@@ -505,7 +506,7 @@ bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
m_pair_sp = pair_sp->GetChildAtIndex(2);
}
- return false;
+ return CacheState::Invalid;
}
size_t lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
@@ -600,22 +601,23 @@ lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::GetChildAtIndex(
return lldb::ValueObjectSP();
}
-bool lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::Update() {
+SyntheticChildrenFrontEnd::CacheState
+lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::Update() {
m_cntrl = nullptr;
ValueObjectSP valobj_sp = m_backend.GetSP();
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
TargetSP target_sp(valobj_sp->GetTargetSP());
if (!target_sp)
- return false;
+ return CacheState::Invalid;
lldb::ValueObjectSP cntrl_sp(valobj_sp->GetChildMemberWithName("__cntrl_"));
m_cntrl = cntrl_sp.get(); // need to store the raw pointer to avoid a circular
// dependency
- return false;
+ return CacheState::Invalid;
}
bool lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
@@ -689,14 +691,15 @@ lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::GetChildAtIndex(
return lldb::ValueObjectSP();
}
-bool lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::Update() {
+SyntheticChildrenFrontEnd::CacheState
+lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::Update() {
ValueObjectSP valobj_sp = m_backend.GetSP();
if (!valobj_sp)
- return false;
+ return CacheState::Invalid;
ValueObjectSP ptr_sp(valobj_sp->GetChildMemberWithName("__ptr_"));
if (!ptr_sp)
- return false;
+ return CacheState::Invalid;
// Retri...
[truncated]
|
Michael137
force-pushed
the
lldb/nfc/synthetic-frontend-udpate-return-type
branch
from
921a207 to
a899377
Compare
kastiglione
reviewed
Jan 31, 2024
| // previously and cached if =false, ValueObjectSyntheticFilter must throw | ||
| // away its cache, and query again for children | ||
| virtual bool Update() = 0; | ||
| enum class CacheState { Invalid, Valid }; |
There was a problem hiding this comment.
what do you think about making this a verb rather than a noun? Something along the lines of CacheAction with cases Clear/Drop/ThrowAway/etc, and Keep/Preserve/Reuse/etc?
There was a problem hiding this comment.
I like Drop/Preserve, that would make the intent clearer.
Any objections @jimingham @clayborg @adrian-prantl ?
There was a problem hiding this comment.
I don't have any better suggestion for a name.
There was a problem hiding this comment.
We need to make sure enum values are 0 and 1 since the python synthetic stuff will return True or False still. It might be nice to move this enum into lldb-enumerations.h so that python can use it in future synthetic child providers, or people can update their synthetic python plugins to use the enum to be more readable.
As far as names go, do we want to be more clear about what these do and specify they are for children? Something like:
/// An enumeration that specifies if children need to be re-computed after a call to Update().
enum class ChildCacheState {
Dynamic = 0, ///< Children need to be recomputed dynamically.
Constant = 1, ///< Children will never change and don't need to be recomputed.
};
There was a problem hiding this comment.
We need to make sure enum values are 0 and 1 since the python synthetic stuff will return True or False still. It might be nice to move this enum into lldb-enumerations.h so that python can use it in future synthetic child providers, or people can update their synthetic python plugins to use the enum to be more readable.
Will do, thanks!
do we want to be more clear about what these do and specify they are for children?
I don't mind these names, though new readers might have trouble understanding (and might be harder to remember which one to use). But being more precise is probably better.
There was a problem hiding this comment.
Any names that at least tell us what they do more clearly will help. I just tried to come up with something off the top of my head...
There was a problem hiding this comment.
As long as we document with headerdoc what each value means, I am good. Then users can use code navigation to take them to the definition and find the explanation.
kastiglione
reviewed
Feb 1, 2024
|
I agree with Dave that verbs would be more naturally follow the intent of the return. Since the return is telling the caller: either "You can reuse the children you have cached" or "you must refetch the children". So if (something.Update() == ChildCacheState::eRefetch) { That seems pretty natural. |
|
I like the |
Michael137
force-pushed
the
lldb/nfc/synthetic-frontend-udpate-return-type
branch
from
793eefc to
6ea87d7
Compare
Michael137
force-pushed
the
lldb/nfc/synthetic-frontend-udpate-return-type
branch
from
6ea87d7 to
df32657
Compare
kastiglione
reviewed
Feb 1, 2024
| /// after a call to \ref SyntheticChildrenFrontEnd::Update. | ||
| enum class ChildCacheState { | ||
| eRefetch = 0, ///< Children need to be recomputed dynamically. | ||
| eReuse = 1, ///< Children will never change and don't need to be recomputed. |
There was a problem hiding this comment.
is "will never change" true in all cases? Or should it be "did not change"?
There was a problem hiding this comment.
After thinking about this, I believe you are correct, it probably means "you can re-use what I gave the last time you called update"
|
ping |
|
Will land after #80609 to not disrupt that patch |
clayborg
approved these changes
Feb 5, 2024
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Feb 16, 2024
…eturn an enum (llvm#80167) This patch changes the return value of `SyntheticChildrenFrontend::Update` to a scoped enum that aims to describe what the return value means. (cherry picked from commit d7fb94b)
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Feb 16, 2024
…eturn an enum (llvm#80167) This patch changes the return value of `SyntheticChildrenFrontend::Update` to a scoped enum that aims to describe what the return value means. (cherry picked from commit d7fb94b)
|
@Michael137 I'm a little late to the party but I just noticed that |
No particular reason apart from it being scoped, which I thought we might prefer for new enums. But there's no reason it couldn't be an old-school enum |
If that's the case I think consistency is more important. Mind putting up a PR? |
Yup will do sometime this week |
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Apr 15, 2024
Done for consistency with the rest of the enums in `lldb-enumerations.h`. See llvm#80167 (comment)
Michael137
added a commit
that referenced
this pull request
Apr 15, 2024
Done for consistency with the rest of the enums in `lldb-enumerations.h`. See #80167 (comment)
aniplcc
pushed a commit
to aniplcc/llvm-project
that referenced
this pull request
Apr 15, 2024
Done for consistency with the rest of the enums in `lldb-enumerations.h`. See llvm#80167 (comment)
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.
This patch changes the return value of
SyntheticChildrenFrontend::Updateto a scoped enum that aims to describe what the return value means.