Change GetNumChildren()/CalculateNumChildren() methods return llvm::Expected #84219
Conversation
This file contains 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
adrian-prantl
requested review from
clayborg,
Michael137 and
bulbazord
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: Adrian Prantl (adrian-prantl) ChangesChange GetNumChildren()/CalculateNumChilrem() methods return llvm::Expected This is an NFC change that does not yet add any error handling or change any code to return any errors. This is the second big change in the patch series started with #83501 A follow-up PR will wire up error handling. Patch is 109.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/84219.diff 71 Files Affected:
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
index 4c0b0b2dae6cd4..652bbef0516eca 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -465,7 +465,7 @@ class ValueObject {
/// Returns a unique id for this ValueObject.
lldb::user_id_t GetID() const { return m_id.GetID(); }
- virtual lldb::ValueObjectSP GetChildAtIndex(size_t idx,
+ virtual lldb::ValueObjectSP GetChildAtIndex(uint32_t idx,
bool can_create = true);
// The method always creates missing children in the path, if necessary.
@@ -476,7 +476,7 @@ class ValueObject {
virtual size_t GetIndexOfChildWithName(llvm::StringRef name);
- size_t GetNumChildren(uint32_t max = UINT32_MAX);
+ llvm::Expected<uint32_t> GetNumChildren(uint32_t max = UINT32_MAX);
const Value &GetValue() const { return m_value; }
@@ -791,7 +791,7 @@ class ValueObject {
return (m_children.find(idx) != m_children.end());
}
- ValueObject *GetChildAtIndex(size_t idx) {
+ ValueObject *GetChildAtIndex(uint32_t idx) {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
const auto iter = m_children.find(idx);
return ((iter == m_children.end()) ? nullptr : iter->second);
@@ -958,9 +958,10 @@ class ValueObject {
int32_t synthetic_index);
/// Should only be called by ValueObject::GetNumChildren().
- virtual size_t CalculateNumChildren(uint32_t max = UINT32_MAX) = 0;
+ virtual llvm::Expected<uint32_t>
+ CalculateNumChildren(uint32_t max = UINT32_MAX) = 0;
- void SetNumChildren(size_t num_children);
+ void SetNumChildren(uint32_t num_children);
void SetValueDidChange(bool value_changed) {
m_flags.m_value_did_change = value_changed;
diff --git a/lldb/include/lldb/Core/ValueObjectCast.h b/lldb/include/lldb/Core/ValueObjectCast.h
index fe053c12d9c343..ba25e166f32688 100644
--- a/lldb/include/lldb/Core/ValueObjectCast.h
+++ b/lldb/include/lldb/Core/ValueObjectCast.h
@@ -33,7 +33,7 @@ class ValueObjectCast : public ValueObject {
std::optional<uint64_t> GetByteSize() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
lldb::ValueType GetValueType() const override;
diff --git a/lldb/include/lldb/Core/ValueObjectChild.h b/lldb/include/lldb/Core/ValueObjectChild.h
index 46b14e6840f0dc..1f88e607cb5737 100644
--- a/lldb/include/lldb/Core/ValueObjectChild.h
+++ b/lldb/include/lldb/Core/ValueObjectChild.h
@@ -39,7 +39,7 @@ class ValueObjectChild : public ValueObject {
lldb::ValueType GetValueType() const override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
ConstString GetTypeName() override;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResult.h b/lldb/include/lldb/Core/ValueObjectConstResult.h
index d61df859bebce4..37dc0867f26c9e 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResult.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResult.h
@@ -67,7 +67,7 @@ class ValueObjectConstResult : public ValueObject {
lldb::ValueType GetValueType() const override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
ConstString GetTypeName() override;
diff --git a/lldb/include/lldb/Core/ValueObjectDynamicValue.h b/lldb/include/lldb/Core/ValueObjectDynamicValue.h
index 2758b4e5bb564d..82c20eee0cd42d 100644
--- a/lldb/include/lldb/Core/ValueObjectDynamicValue.h
+++ b/lldb/include/lldb/Core/ValueObjectDynamicValue.h
@@ -43,7 +43,7 @@ class ValueObjectDynamicValue : public ValueObject {
ConstString GetDisplayTypeName() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
lldb::ValueType GetValueType() const override;
diff --git a/lldb/include/lldb/Core/ValueObjectMemory.h b/lldb/include/lldb/Core/ValueObjectMemory.h
index 3c01df388d2e6d..a8fb0353d601b2 100644
--- a/lldb/include/lldb/Core/ValueObjectMemory.h
+++ b/lldb/include/lldb/Core/ValueObjectMemory.h
@@ -47,7 +47,7 @@ class ValueObjectMemory : public ValueObject {
ConstString GetDisplayTypeName() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
lldb::ValueType GetValueType() const override;
diff --git a/lldb/include/lldb/Core/ValueObjectRegister.h b/lldb/include/lldb/Core/ValueObjectRegister.h
index 2e47eee3d7f793..fec8566ba33d90 100644
--- a/lldb/include/lldb/Core/ValueObjectRegister.h
+++ b/lldb/include/lldb/Core/ValueObjectRegister.h
@@ -47,7 +47,7 @@ class ValueObjectRegisterSet : public ValueObject {
ConstString GetQualifiedTypeName() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
int32_t synthetic_index) override;
@@ -95,7 +95,7 @@ class ValueObjectRegister : public ValueObject {
ConstString GetTypeName() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
bool SetValueFromCString(const char *value_str, Status &error) override;
diff --git a/lldb/include/lldb/Core/ValueObjectSyntheticFilter.h b/lldb/include/lldb/Core/ValueObjectSyntheticFilter.h
index 67596232eafd1e..ca6d6c728005db 100644
--- a/lldb/include/lldb/Core/ValueObjectSyntheticFilter.h
+++ b/lldb/include/lldb/Core/ValueObjectSyntheticFilter.h
@@ -47,11 +47,11 @@ class ValueObjectSynthetic : public ValueObject {
bool MightHaveChildren() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
lldb::ValueType GetValueType() const override;
- lldb::ValueObjectSP GetChildAtIndex(size_t idx,
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx,
bool can_create = true) override;
lldb::ValueObjectSP GetChildMemberWithName(llvm::StringRef name,
diff --git a/lldb/include/lldb/Core/ValueObjectVTable.h b/lldb/include/lldb/Core/ValueObjectVTable.h
index 217ff8d0d334ce..4662f395a4dde9 100644
--- a/lldb/include/lldb/Core/ValueObjectVTable.h
+++ b/lldb/include/lldb/Core/ValueObjectVTable.h
@@ -64,7 +64,7 @@ class ValueObjectVTable : public ValueObject {
std::optional<uint64_t> GetByteSize() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
int32_t synthetic_index) override;
diff --git a/lldb/include/lldb/Core/ValueObjectVariable.h b/lldb/include/lldb/Core/ValueObjectVariable.h
index bba28ce567b2a0..db3847f14a0b5a 100644
--- a/lldb/include/lldb/Core/ValueObjectVariable.h
+++ b/lldb/include/lldb/Core/ValueObjectVariable.h
@@ -46,7 +46,7 @@ class ValueObjectVariable : public ValueObject {
ConstString GetDisplayTypeName() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
lldb::ValueType GetValueType() const override;
diff --git a/lldb/include/lldb/DataFormatters/TypeSynthetic.h b/lldb/include/lldb/DataFormatters/TypeSynthetic.h
index 23cc054b399a67..1c47b32c0ecc54 100644
--- a/lldb/include/lldb/DataFormatters/TypeSynthetic.h
+++ b/lldb/include/lldb/DataFormatters/TypeSynthetic.h
@@ -38,14 +38,16 @@ class SyntheticChildrenFrontEnd {
virtual ~SyntheticChildrenFrontEnd() = default;
- virtual size_t CalculateNumChildren() = 0;
+ virtual llvm::Expected<uint32_t> CalculateNumChildren() = 0;
- virtual size_t CalculateNumChildren(uint32_t max) {
+ virtual llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) {
auto count = CalculateNumChildren();
- return count <= max ? count : max;
+ if (!count)
+ return count;
+ return *count <= max ? *count : max;
}
- virtual lldb::ValueObjectSP GetChildAtIndex(size_t idx) = 0;
+ virtual lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) = 0;
virtual size_t GetIndexOfChildWithName(ConstString name) = 0;
@@ -109,9 +111,9 @@ class SyntheticValueProviderFrontEnd : public SyntheticChildrenFrontEnd {
~SyntheticValueProviderFrontEnd() override = default;
- size_t CalculateNumChildren() override { return 0; }
+ llvm::Expected<uint32_t> CalculateNumChildren() override { return 0; }
- lldb::ValueObjectSP GetChildAtIndex(size_t idx) override { return nullptr; }
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override { return nullptr; }
size_t GetIndexOfChildWithName(ConstString name) override {
return UINT32_MAX;
@@ -322,9 +324,11 @@ class TypeFilterImpl : public SyntheticChildren {
~FrontEnd() override = default;
- size_t CalculateNumChildren() override { return filter->GetCount(); }
+ llvm::Expected<uint32_t> CalculateNumChildren() override {
+ return filter->GetCount();
+ }
- lldb::ValueObjectSP GetChildAtIndex(size_t idx) override {
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override {
if (idx >= filter->GetCount())
return lldb::ValueObjectSP();
return m_backend.GetSyntheticExpressionPathChild(
@@ -426,11 +430,11 @@ class ScriptedSyntheticChildren : public SyntheticChildren {
bool IsValid();
- size_t CalculateNumChildren() override;
+ llvm::Expected<uint32_t> CalculateNumChildren() override;
- size_t CalculateNumChildren(uint32_t max) override;
+ llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
- lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override;
lldb::ChildCacheState Update() override;
diff --git a/lldb/include/lldb/DataFormatters/VectorIterator.h b/lldb/include/lldb/DataFormatters/VectorIterator.h
index 5f774bb72c3a3a..70bcf50ca1b1d2 100644
--- a/lldb/include/lldb/DataFormatters/VectorIterator.h
+++ b/lldb/include/lldb/DataFormatters/VectorIterator.h
@@ -24,9 +24,9 @@ class VectorIteratorSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
VectorIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp,
llvm::ArrayRef<ConstString> item_names);
- size_t CalculateNumChildren() override;
+ llvm::Expected<uint32_t> CalculateNumChildren() override;
- lldb::ValueObjectSP GetChildAtIndex(size_t idx) override;
+ lldb::ValueObjectSP GetChildAtIndex(uint32_t idx) override;
lldb::ChildCacheState Update() override;
diff --git a/lldb/include/lldb/Symbol/CompilerType.h b/lldb/include/lldb/Symbol/CompilerType.h
index 414c44275aaafc..c1dce4ccbf79c2 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -386,8 +386,9 @@ class CompilerType {
std::optional<size_t> GetTypeBitAlign(ExecutionContextScope *exe_scope) const;
- uint32_t GetNumChildren(bool omit_empty_base_classes,
- const ExecutionContext *exe_ctx) const;
+ llvm::Expected<uint32_t>
+ GetNumChildren(bool omit_empty_base_classes,
+ const ExecutionContext *exe_ctx) const;
lldb::BasicType GetBasicTypeEnumeration() const;
diff --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h
index acd1a769f13cd6..b5eac5fa732d67 100644
--- a/lldb/include/lldb/Symbol/Type.h
+++ b/lldb/include/lldb/Symbol/Type.h
@@ -440,7 +440,7 @@ class Type : public std::enable_shared_from_this<Type>, public UserID {
std::optional<uint64_t> GetByteSize(ExecutionContextScope *exe_scope);
- uint32_t GetNumChildren(bool omit_empty_base_classes);
+ llvm::Expected<uint32_t> GetNumChildren(bool omit_empty_base_classes);
bool IsAggregateType();
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h b/lldb/include/lldb/Symbol/TypeSystem.h
index 63829131556e87..f647fcbf1636ea 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -300,9 +300,10 @@ class TypeSystem : public PluginInterface,
virtual lldb::Format GetFormat(lldb::opaque_compiler_type_t type) = 0;
- virtual uint32_t GetNumChildren(lldb::opaque_compiler_type_t type,
- bool omit_empty_base_classes,
- const ExecutionContext *exe_ctx) = 0;
+ virtual llvm::Expected<uint32_t>
+ GetNumChildren(lldb::opaque_compiler_type_t type,
+ bool omit_empty_base_classes,
+ const ExecutionContext *exe_ctx) = 0;
virtual CompilerType GetBuiltinTypeByName(ConstString name);
diff --git a/lldb/include/lldb/Target/StackFrameRecognizer.h b/lldb/include/lldb/Target/StackFrameRecognizer.h
index 419f0c0aac1f86..5e8e12b2a4e961 100644
--- a/lldb/include/lldb/Target/StackFrameRecognizer.h
+++ b/lldb/include/lldb/Target/StackFrameRecognizer.h
@@ -164,7 +164,8 @@ class ValueObjectRecognizerSynthesizedValue : public ValueObject {
m_value = m_parent->GetValue();
return true;
}
- size_t CalculateNumChildren(uint32_t max = UINT32_MAX) override {
+ llvm::Expected<uint32_t>
+ CalculateNumChildren(uint32_t max = UINT32_MAX) override {
return m_parent->GetNumChildren(max);
}
CompilerType GetCompilerTypeImpl() override {
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 89d26a1fbe2824..6668f642ab2ef5 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -947,7 +947,8 @@ uint32_t SBValue::GetNumChildren(uint32_t max) {
ValueLocker locker;
lldb::ValueObjectSP value_sp(GetSP(locker));
if (value_sp)
- num_children = value_sp->GetNumChildren(max);
+ num_children =
+ llvm::expectedToStdOptional(value_sp->GetNumChildren(max)).value_or(0);
return num_children;
}
diff --git a/lldb/source/Core/FormatEntity.cpp b/lldb/source/Core/FormatEntity.cpp
index fa5eadc6ff4e9a..5e8cd2e1dda39f 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -926,7 +926,8 @@ static bool DumpValue(Stream &s, const SymbolContext *sc,
s.PutChar('[');
if (index_higher < 0)
- index_higher = valobj->GetNumChildren() - 1;
+ index_higher =
+ llvm::expectedToStdOptional(valobj->GetNumChildren()).value_or(0) - 1;
uint32_t max_num_children =
target->GetTargetSP()->GetMaximumNumberOfChildrenToDisplay();
diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 620e68a28510ef..dcb83aa692a1c1 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -4519,7 +4519,8 @@ struct Row {
calculated_children = true;
ValueObjectSP valobj = value.GetSP();
if (valobj) {
- const size_t num_children = valobj->GetNumChildren();
+ const size_t num_children =
+ llvm::expectedToStdOptional(valobj->GetNumChildren()).value_or(0);
for (size_t i = 0; i < num_children; ++i) {
children.push_back(Row(valobj->GetChildAtIndex(i), this));
}
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 840b100c70ddaa..03fb3c86d89394 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -372,12 +372,12 @@ bool ValueObject::IsLogicalTrue(Status &error) {
return ret;
}
-ValueObjectSP ValueObject::GetChildAtIndex(size_t idx, bool can_create) {
+ValueObjectSP ValueObject::GetChildAtIndex(uint32_t idx, bool can_create) {
ValueObjectSP child_sp;
// We may need to update our value if we are dynamic
if (IsPossibleDynamicType())
UpdateValueIfNeeded(false);
- if (idx < GetNumChildren()) {
+ if (idx < llvm::expectedToStdOptional(GetNumChildren()).value_or(0)) {
// Check if we have already made the child value object?
if (can_create && !m_children.HasChildAtIndex(idx)) {
// No we haven't created the child at this index, so lets have our
@@ -440,7 +440,7 @@ ValueObjectSP ValueObject::GetChildMemberWithName(llvm::StringRef name,
return child_sp;
}
-size_t ValueObject::GetNumChildren(uint32_t max) {
+llvm::Expected<uint32_t> ValueObject::GetNumChildren(uint32_t max) {
UpdateValueIfNeeded();
if (max < UINT32_MAX) {
@@ -452,7 +452,11 @@ size_t ValueObject::GetNumChildren(uint32_t max) {
}
if (!m_flags.m_children_count_valid) {
- SetNumChildren(CalculateNumChildren());
+ auto num_children_or_err = CalculateNumChildren();
+ if (num_children_or_err)
+ SetNumChildren(*num_children_or_err);
+ else
+ return num_children_or_err;
}
return m_children.GetChildrenCount();
}
@@ -464,13 +468,14 @@ bool ValueObject::MightHaveChildren() {
if (type_info & (eTypeHasChildren | eTypeIsPointer | eTypeIsReference))
has_children = true;
} else {
- has_children = GetNumChildren() > 0;
+ has_children =
+ llvm::expectedToStdOptional(GetNumChildren()).value_or(0) > 0;
}
return has_children;
}
// Should only be called by ValueObject::GetNumChildren()
-void ValueObject::SetNumChildren(size_t num_children) {
+void ValueObject::SetNumChildren(uint32_t num_children) {
m_flags.m_children_count_valid = true;
m_children.SetChildrenCount(num_children);
}
@@ -1176,7 +1181,8 @@ bool ValueObject::DumpPrintableRepresentation(
if (flags.Test(eTypeIsArray)) {
if ((custom_format == eFormatBytes) ||
(custom_format == eFormatBytesWithASCII)) {
- const size_t count = GetNumChildren();
+ const size_t count =
+ llvm::expectedToStdOptional(GetNumChildren()).value_or(0);
s << '[';
for (size_t low = 0; low < count; low++) {
@@ -1215,7 +1221,8 @@ bool ValueObject::DumpPrintableRepresentation(
// format should be printed
// directly
{
- const size_t count = GetNumChildren();
+ const size_t count =
+ llvm::expectedToStdOptional(GetNumChildren()).value_or(0);
Format format = FormatManager::GetSingleItemFormat(custom_format);
@@ -1294,7 +1301,9 @@ bool ValueObject::DumpPrintableRepresentation(
break;
case eValueObjectRepresentationStyleChildrenCount:
- strm.Printf("%" PRIu64 "", (uint64_t)GetNumChildren());
+ strm.Printf(
+ "%" PRIu64 "",
+ (uint64_t)llvm::expectedToStdOptional(GetNumChildren()).value_or(0));
str = strm.GetString();
break;
@@ -2320,7 +2329,9 @@ ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
child_valobj_sp = root->GetSyntheticArrayMember(index, true);
if (!child_valobj_sp)
if (root->HasSyntheticValue() &&
- root->GetSyntheticValue()->GetNumChildren() > index)
+ llvm::expectedToStdOptional(
+ root->GetSyntheticValue()->GetNumChildren())
+ .value_or(0) > index)
child_valobj_sp =
root->GetSyntheticValue()->GetChildAtIndex(index);
if (child_valobj_sp) {
diff --git a/lldb/source/Core/ValueObjectCast.cpp b/lldb/source/Core/ValueObjectCast.cpp
index 0882d4b3677619..c8e31641514170 100644
--- a/lldb/source/Core/ValueObjectCast.cpp
+++ b/lldb/source/Core/ValueObjectCast.cpp
@@ -41,11 +41,13 @@ ValueObjectCast::~ValueObjectCast() = default;
CompilerType ValueObjectCast::GetCompilerTypeImpl() { return m_cast_type; }
-size_t ValueObjectCast::CalculateNumChildren(uint32_t max) {
+llvm::Expected<uint32_t> ValueObjectCast::CalculateNumChildren(uint32_t max) {
ExecutionContext exe_ctx(GetExecutionContextRef());
auto children_count = GetCompilerType().GetNumChildren(
true, &exe_ctx);
- return children_count <= max ? children_count : max;
+ if (!children_count)
+ return children_count;
+ return *children_count <= max ? *children_count : max;
}
std::optional<uint64_t> ValueObjectCast::GetByteSize() {
diff --git a/lldb/source/Core/ValueObjectChild.cpp b/lldb/source/Core/Va...
[truncated]
|
adrian-prantl
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
adrian-prantl
force-pushed
the
expectednumchildren
branch
from
d3112e0
to
6231f11
Compare
|
@walter-erquinigo I was wondering if it would make sense to create a DropAndLogError() wrapper for all these places. The main problem would be that we end up calling these functions dozens of times over and over so we'd be spamming the logs quite a bit. But maybe doing a verbose log would be appropriate. I'll modify the patch accordingly. |
|
@adrian-prantl , I like that idea. Dump the error messages only to the formatters channel in verbose mode. |
adrian-prantl
force-pushed
the
expectednumchildren
branch
from
6231f11
to
cc079ca
Compare
JDevlieghere
requested changes
Mar 7, 2024
There was a problem hiding this comment.
I'm worried about making it too easy to ignore errors. The whole point of the llvm::Error class is that you have to check it. There might be a legitimate reason you don't about the error the call site, but it should very explicit and easily auditable. If none of the callers care about the error, then it shouldn't be an Error/Expected in the first place. We have plenty of examples of that in LLVM.
I totally understand the need for this as a transitional tool, but I want to make sure nobody writes new code using this. I can see two ways to make discourage that:
- Mark the helper as deprecated. This might generate a lot off noise though.
- Make it really obvious in the name that this is a temporary or that we're purposely dropping the error. Either something like
FIX_ERROR_HANDLINGorLLDB_DROP_ERRORor something. Logging to the verbose log is better than nothing, but it doesn't constitute error handling.
PS: The current implementation wraps the LLDB_LOG_* which uses #line and #file information. Now all those errors will get attributed to this helper function.
|
I would say this differently. Many clients of the GetNumChildren API are planning to respond the same way to an error and a number of children == 0. Having to embed this "i'm not actually checking the error, I'm just converting it to num-children = 0" snippet looks odd, like "Why are you bothering with errors when you just discard them immediately for "value is 0".
A better title for this use of GetNumChildren would be something like GetNumChildrenErrorsAreZero. That has the virtue of saying what's going on explicitly.
Jim
… On Mar 7, 2024, at 10:45 AM, Jonas Devlieghere ***@***.***> wrote:
@JDevlieghere requested changes on this pull request.
I'm worried about making it too easy to ignore errors. The whole point of the llvm::Error class is that you have to check it. There might be a legitimate reason you don't about the error the call site, but it should very explicit and easily auditable. If none of the callers care about the error, then it shouldn't be an Error/Expected in the first place. We have plenty of examples of that in LLVM.
I totally understand the need for this as a transitional tool, but I want to make sure nobody writes new code using this. I can see two ways to make discourage that:
Mark the helper as deprecated. This might generate a lot off noise though.
Make it really obvious in the name that this is a temporary or that we're purposely dropping the error. Either something like FIX_ERROR_HANDLING or LLDB_DROP_ERROR or something. Logging to the verbose log is better than nothing, but it doesn't constitute error handling.
PS: The current implementation wraps the LLDB_LOG_* which uses #line and #file information. Now all those errors will get attributed to this helper function.
—
Reply to this email directly, view it on GitHub <#84219 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW6WTDMOLUAJSNVWUILYXCYVNAVCNFSM6AAAAABEJS3ASKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRTGI2TCMZTGE>.
You are receiving this because you are on a team that was mentioned.
|
I haven't done a complete audit, but my expectation for this patch is that the vast majority of users of this API will want to bubble up the error. So I'm seeing this as a transition state, with the goal being to convert all functions that currently use the ValueOr function to also return an Expected. |
Cool! If that ends up not being the case then it would be good to have something that says the distinction doesn't matter, but if this construct vanishes, all the better! |
adrian-prantl
force-pushed
the
expectednumchildren
branch
from
cc079ca
to
97a80ff
Compare
|
@walter-erquinigo @jimingham I updated the patch to implement Jim's suggestion of adding an explicit |
jimingham
approved these changes
Mar 8, 2024
This is an NFC change that does not yet add any error handling or change any code to return any errors. Some select APIs may legitimately not care about errors, so this patch also adds a GetNumChildrenIgnoringErrors() variant to the API. As we convert more APIs to returning llvm::Expected, we expect the number of calls to GetNumChildrenIgnoringErrors() to dwindle.
adrian-prantl
force-pushed
the
expectednumchildren
branch
from
97a80ff
to
0504cd6
Compare
|
I think this broke the build completely? |
|
Apologies, I made a last-minute rebase without rebuilding. Thanks for reverting! |
|
Or rather, I didn't build with all LLVM targets. |
adrian-prantl
added a commit
that referenced
this pull request
Mar 9, 2024
…xpected (#84219) Change GetNumChildren()/CalculateNumChildren() methods return llvm::Expected This is an NFC change that does not yet add any error handling or change any code to return any errors. This is the second big change in the patch series started with #83501 A follow-up PR will wire up error handling.
adrian-prantl
added a commit
that referenced
this pull request
Mar 11, 2024
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Mar 11, 2024
…xpected (llvm#84219) Change GetNumChildren()/CalculateNumChildren() methods return llvm::Expected This is an NFC change that does not yet add any error handling or change any code to return any errors. This is the second big change in the patch series started with llvm#83501 A follow-up PR will wire up error handling. (cherry picked from commit 624ea68)
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Mar 11, 2024
This is a proof-of-concept patch that illustrates how to use the Expected return values to surface rich error messages all the way up to the ValueObjectPrinter. This is the final patch in the series that includes llvm#83501 and llvm#84219 (cherry picked from commit 6462ead)
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.
Change GetNumChildren()/CalculateNumChildren() methods return llvm::Expected
This is an NFC change that does not yet add any error handling or change any code to return any errors.
This is the second big change in the patch series started with #83501
A follow-up PR will wire up error handling.