Fix weak symbol linkage in SBStructuredData, update docs.

[tfiala]

Summary

This change fixes an issue where I was leaking a weakly-linked symbol in the SBAPI. It also updates the docs to call out what I did wrong.

[jingham]

This is fine with one comment on the text.

SB-api-coding-rules.html

51–54 ↗

(On Diff #77389)

I think it's more straightforward to say:

Please note that you should not put this Impl class in the lldb namespace. Failure to do so...

After all, it would be equally bad to have put it inside the "namespace lldb" but not in the class.

[tfiala]

Making changes to the text...

SB-api-coding-rules.html

51–54 ↗

(On Diff #77389)

Okay, I can use your verbiage. The end of one sentence did explicitly call out having it not be in the public lldb namespace:

"...but rather should be a separate class that is not present in the public lldb namespace."

but I think your prose is clearer, though. Thank you!

[tfiala]

Adjusted web docs.

[jingham]

Looks good.

[tfiala]

Thanks, Jim!

lldb/trunk/include/lldb/API/SBStructuredData.h

 //===-- SBStructuredData.h --------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SBStructuredData_h
 #define SBStructuredData_h
 
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBModule.h"
 
+class StructuredDataImpl;
+
 namespace lldb {
 
 class SBStructuredData {
 public:
   SBStructuredData();
 
   SBStructuredData(const lldb::SBStructuredData &rhs);
 
   SBStructuredData(const lldb::EventSP &event_sp);
 
   ~SBStructuredData();
 
   lldb::SBStructuredData &operator=(const lldb::SBStructuredData &rhs);
 
   bool IsValid() const;
 
   void Clear();
 
   lldb::SBError GetAsJSON(lldb::SBStream &stream) const;
 
   lldb::SBError GetDescription(lldb::SBStream &stream) const;
 
 private:
-  class Impl;
-  std::unique_ptr<Impl> m_impl_up;
+  std::unique_ptr<StructuredDataImpl> m_impl_up;
 };
 }
 
 #endif /* SBStructuredData_h */

lldb/trunk/source/API/SBStructuredData.cpp

 #include "lldb/Core/Stream.h"
 #include "lldb/Core/StructuredData.h"
 #include "lldb/Target/StructuredDataPlugin.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 #pragma mark--
-#pragma mark Impl
+#pragma mark StructuredDataImpl
 
-class SBStructuredData::Impl {
+class StructuredDataImpl {
 public:
-  Impl() : m_plugin_wp(), m_data_sp() {}
+  StructuredDataImpl() : m_plugin_wp(), m_data_sp() {}
 
-  Impl(const Impl &rhs) = default;
+  StructuredDataImpl(const StructuredDataImpl &rhs) = default;
 
-  Impl(const EventSP &event_sp)
+  StructuredDataImpl(const EventSP &event_sp)
       : m_plugin_wp(
             EventDataStructuredData::GetPluginFromEvent(event_sp.get())),
         m_data_sp(EventDataStructuredData::GetObjectFromEvent(event_sp.get())) {
   }
 
-  ~Impl() = default;
+  ~StructuredDataImpl() = default;
 
-  Impl &operator=(const Impl &rhs) = default;
+  StructuredDataImpl &operator=(const StructuredDataImpl &rhs) = default;
 
   bool IsValid() const { return m_data_sp.get() != nullptr; }
 
   void Clear() {
     m_plugin_wp.reset();
     m_data_sp.reset();
   }
 
-  SBError GetAsJSON(lldb::SBStream &stream) const {
+  SBError GetAsJSON(lldb_private::Stream &stream) const {
     SBError sb_error;
 
     if (!m_data_sp) {
       sb_error.SetErrorString("No structured data.");
       return sb_error;
     }
 
-    m_data_sp->Dump(stream.ref());
+    m_data_sp->Dump(stream);
     return sb_error;
   }
 
-  lldb::SBError GetDescription(lldb::SBStream &stream) const {
-    SBError sb_error;
+  Error GetDescription(lldb_private::Stream &stream) const {
+    Error error;
 
     if (!m_data_sp) {
-      sb_error.SetErrorString("Cannot pretty print structured data: "
-                              "no data to print.");
-      return sb_error;
+      error.SetErrorString("Cannot pretty print structured data: "
+      return error;
     }
 
     // Grab the plugin.
     auto plugin_sp = StructuredDataPluginSP(m_plugin_wp);
     if (!plugin_sp) {
-      sb_error.SetErrorString("Cannot pretty print structured data: "
-                              "plugin doesn't exist.");
-      return sb_error;
+      error.SetErrorString("Cannot pretty print structured data: "
+      return error;
     }
 
     // Get the data's description.
-    auto error = plugin_sp->GetDescription(m_data_sp, stream.ref());
-    if (!error.Success())
-      sb_error.SetError(error);
-
-    return sb_error;
+    return plugin_sp->GetDescription(m_data_sp, stream);
   }
 
 private:
   StructuredDataPluginWP m_plugin_wp;
   StructuredData::ObjectSP m_data_sp;
 };
 
 #pragma mark--
 #pragma mark SBStructuredData
 
-SBStructuredData::SBStructuredData() : m_impl_up(new Impl()) {}
+SBStructuredData::SBStructuredData() : m_impl_up(new StructuredDataImpl()) {}
 
 SBStructuredData::SBStructuredData(const lldb::SBStructuredData &rhs)
-    : m_impl_up(new Impl(*rhs.m_impl_up.get())) {}
+    : m_impl_up(new StructuredDataImpl(*rhs.m_impl_up.get())) {}
 
 SBStructuredData::SBStructuredData(const lldb::EventSP &event_sp)
-    : m_impl_up(new Impl(event_sp)) {}
+    : m_impl_up(new StructuredDataImpl(event_sp)) {}
 
 SBStructuredData::~SBStructuredData() {}
 
 SBStructuredData &SBStructuredData::
 operator=(const lldb::SBStructuredData &rhs) {
   *m_impl_up = *rhs.m_impl_up;
   return *this;
 }
 
 bool SBStructuredData::IsValid() const { return m_impl_up->IsValid(); }
 
 void SBStructuredData::Clear() { m_impl_up->Clear(); }
 
 SBError SBStructuredData::GetAsJSON(lldb::SBStream &stream) const {
-  return m_impl_up->GetAsJSON(stream);
+  return m_impl_up->GetAsJSON(stream.ref());
 }
 
 lldb::SBError SBStructuredData::GetDescription(lldb::SBStream &stream) const {
-  return m_impl_up->GetDescription(stream);
+  Error error = m_impl_up->GetDescription(stream.ref());
+  SBError sb_error;
+  sb_error.SetError(error);
+  return sb_error;
 }

lldb/trunk/www/SB-api-coding-rules.html

<div id="middle">
                                       
                                       <p>All the SB API classes are non-virtual, single inheritance classes.  They should only include
                                         SBDefines.h or other SB headers as needed.  There should be no inlined method implementations
                                         in the header files, they should all be in the implementation files.  And there should be no
                                         direct ivar access.
 
                                       <p>You also need to choose the ivars for the class with care, since you can't add or remove ivars
                                         without breaking binary compatibility.  In some cases, the SB class is a thin wrapper around
-                                        an interal lldb_private object.  In that case, the class can have a single ivar, which is 
+                                        an internal lldb_private object.  In that case, the class can have a single ivar, which is
                                         either a pointer, shared_ptr or unique_ptr to the object in the lldb_private API.  All the
                                         lldb_private classes that get used this way are declared as opaque classes in lldb_forward.h,
                                         which is included in SBDefines.h.  So if you need an SB class to wrap an lldb_private class
                                         that isn't in lldb_forward.h, add it there rather than making a direct opaque declaration in
                                         the SB classes .h file.  
 
                                       <p>If the SB Class needs some state of its own, as well as the backing object, don't include that
                                         as a direct ivar in the SB Class.  Instead, make an Impl class in the SB's .cpp file, and then
                                         make the SB object hold a shared or unique pointer to the Impl object.  The theory behind this is
                                         that if you need more state in the SB object, those needs are likely to change over time, 
-                                        and this way the impl class can pick up members without changing the size of the object.
-                                        An example of this is the SBValue class.
+                                        and this way the Impl class can pick up members without changing the size of the object.
+                                        An example of this is the SBValue class.  Please note that you should not put this Impl class
+                                        in the lldb namespace.  Failure to do so leads to leakage of weak-linked symbols in the SBAPI.
                                         
                                       <p>In order to fit into the Python API's, we need to be able to default construct all the SB objects.
                                         Since the ivars of the classes are all pointers of one sort or other, this can easily be done, but
                                         it means all the methods must be 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.
 
                                       <p>Another piece of the SB API infrastructure is the Python (or other script interpreter) customization.