[Lldb-commits] [lldb] [lldb] Document the behaviour of IsValid for SBError (PR #170862)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett updated https://github.com/llvm/llvm-project/pull/170862

>From afe71b2e714be44515fd31b679ca92e240aab40f Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Fri, 5 Dec 2025 14:07:23 +0000
Subject: [PATCH 1/2] [lldb] Document the behaviour of IsValid for SBError

This reverts commit d20d84fec5945fcc16aa6f63879e1458d4af9ea6.

In which I changed an SBError use so that when the function
succeeded, IsValid on the SBError would be true.

This seemed to make sense but SBError acts differently to
other SB classes in this respect. For something like SBMemoryRegionInfo,
if IsValid() is false, you can't do anything with it.

However for SBError, IsValid() true only means there's some
underlying error object in there. If the SBError represents
a success, there's no need to put anything in there.

You can see this intent from a lot of its methods, they
all have handling for IsValid() false.

This is not a bug but a misunderstanding of what IsValid
means. Yes it does function the way I expected for most
classes, but it does not for SBError and though that's
not intuitive, it is consistent with how we describe
IsValid in the documentation.

So instead of changing that method's use of SBError I'm
documenting this initially counterintuitive behaviour.
---
 lldb/docs/resources/sbapi.rst   | 10 ++++++++++
 lldb/include/lldb/API/SBError.h |  9 +++++++++
 lldb/source/API/SBDebugger.cpp  |  2 +-
 lldb/unittests/DAP/TestBase.cpp |  1 -
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lldb/docs/resources/sbapi.rst b/lldb/docs/resources/sbapi.rst
index 4ca3909e0f291..db596ddc9fbab 100644
--- a/lldb/docs/resources/sbapi.rst
+++ b/lldb/docs/resources/sbapi.rst
@@ -46,6 +46,16 @@ prepared to handle their opaque implementation pointer being empty, and doing
 something reasonable. We also always have an "IsValid" method on all the SB
 classes to report whether the object is empty or not.
 
+.. note:: The implication of an object being "empty" can vary by class.
+
+          For most classes, the lack of anything backing the class means that it
+          would not be valid to interact with it by calling any other methods
+          on it.
+
+          One exception to this is ``SBError``, which can provide valid
+          information even when empty. This is because it does not need an
+          underlying object to be able to represent a success state.
+
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators
 and documentation to classes. We place the property accessors and iterators in
diff --git a/lldb/include/lldb/API/SBError.h b/lldb/include/lldb/API/SBError.h
index 6a4a39c4db314..dd8c0f939775f 100644
--- a/lldb/include/lldb/API/SBError.h
+++ b/lldb/include/lldb/API/SBError.h
@@ -72,6 +72,15 @@ class LLDB_API SBError {
 
   explicit operator bool() const;
 
+  /// \brief Returns \c true if this object contains an underlying \c Status
+  /// object.
+  ///
+  /// That object may represent a success or a failure. When \c IsValid returns
+  /// \c false, it may be the case that the \c SBError represents a success but
+  /// does not contain a \c Status representing that success.
+  ///
+  /// It is safe to call \c Success or \c Fail in the case where \c IsValid
+  /// returns \c false.
   bool IsValid() const;
 
   bool GetDescription(lldb::SBStream &description);
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index f939955ba57c8..7a4bebfdf998e 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -179,7 +179,7 @@ void SBDebugger::Initialize() {
 lldb::SBError SBDebugger::InitializeWithErrorHandling() {
   LLDB_INSTRUMENT();
 
-  SBError error((Status()));
+  SBError error;
   if (auto e = g_debugger_lifetime->Initialize(
           std::make_unique<SystemInitializerFull>())) {
     error.SetError(Status::FromError(std::move(e)));
diff --git a/lldb/unittests/DAP/TestBase.cpp b/lldb/unittests/DAP/TestBase.cpp
index f4dde9559e9d3..8cb459964f7d8 100644
--- a/lldb/unittests/DAP/TestBase.cpp
+++ b/lldb/unittests/DAP/TestBase.cpp
@@ -72,7 +72,6 @@ void DAPTestBase::TearDown() {
 
 void DAPTestBase::SetUpTestSuite() {
   lldb::SBError error = SBDebugger::InitializeWithErrorHandling();
-  EXPECT_TRUE(error.IsValid());
   EXPECT_TRUE(error.Success());
 }
 void DAPTestBase::TeatUpTestSuite() { SBDebugger::Terminate(); }

>From 57fa56dacd02ade23c4611eceeac6e4f3d47689a Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Fri, 5 Dec 2025 14:45:52 +0000
Subject: [PATCH 2/2] formatting

---
 lldb/docs/resources/sbapi.rst | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lldb/docs/resources/sbapi.rst b/lldb/docs/resources/sbapi.rst
index db596ddc9fbab..8c15e371a5012 100644
--- a/lldb/docs/resources/sbapi.rst
+++ b/lldb/docs/resources/sbapi.rst
@@ -46,15 +46,16 @@ prepared to handle their opaque implementation pointer being empty, and doing
 something reasonable. We also always have an "IsValid" method on all the SB
 classes to report whether the object is empty or not.
 
-.. note:: The implication of an object being "empty" can vary by class.
+.. note::
+  The implication of an object being "empty" can vary by class.
 
-          For most classes, the lack of anything backing the class means that it
-          would not be valid to interact with it by calling any other methods
-          on it.
+  For most classes, the lack of anything backing the class means that it
+  would not be valid to interact with it by calling any other methods
+  on it.
 
-          One exception to this is ``SBError``, which can provide valid
-          information even when empty. This is because it does not need an
-          underlying object to be able to represent a success state.
+  One exception to this is ``SBError``, which can provide valid
+  information even when empty. This is because it does not need an
+  underlying object to be able to represent a success state.
 
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators