[Lldb-commits] [PATCH] D115073: Modify DataEncoder to be able to encode data in an object owned buffer.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It seems to me that the DataEncoder class is a lot more complicated than what is necessary for our existing use cases. All of them involve creating a new buffer (either empty, or with some pre-existing content), modifying it, and passing it on. A lot of the DataEncoder complexity (`IsDynamic`, `UpdateMemberVariables`, ...) stems from the fact that it wants to support writing into unowned buffers, functionality that we don't even use.

If we removed that, we could make this code a lot simpler. See inline comments for one way to achieve that.



================
Comment at: lldb/include/lldb/Utility/DataEncoder.h:79-80
+  ///     A size of an address in bytes. \see PutAddress
   DataEncoder(void *data, uint32_t data_length, lldb::ByteOrder byte_order,
               uint8_t addr_size);
 
----------------
Take `const void*data` here and make a copy for internal use inside the constructor.


================
Comment at: lldb/include/lldb/Utility/DataEncoder.h:283-291
+  /// Return if this object has a dynamic buffer that can be appended to.
+  ///
+  /// The buffer is dynamic if the "m_data_sp" is valid. "m_data_sp" is only set
+  /// by constructors mentioned below.
+  ///
+  /// \see DataEncoder(lldb::ByteOrder byte_order, uint8_t addr_size);
+  bool IsDynamic() const {
----------------
delete


================
Comment at: lldb/include/lldb/Utility/DataEncoder.h:309-314
+  /// Update the m_start and m_end after appending data.
   ///
-  /// \param[in] offset
-  ///     The offset into \a data_sp at which the subset starts.
-  ///
-  /// \param[in] length
-  ///     The length in bytes of the subset of \a data_sp.
-  ///
-  /// \return
-  ///     The number of bytes that this object now contains.
-  uint32_t SetData(const lldb::DataBufferSP &data_sp, uint32_t offset = 0,
-                   uint32_t length = UINT32_MAX);
+  /// Any of the member functions that append data to the end of the owned 
+  /// data should call this function after appending data to update the required
+  /// member variables.
+  void UpdateMemberVariables();
----------------
delete, along with the member variables it updates.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:463-465
       // So first we copy the data into a heap buffer
       std::unique_ptr<DataBufferHeap> head_data_up(
           new DataBufferHeap(m_data.GetDataStart(), m_data.GetByteSize()));
----------------
delete, and create data encoder from `m_data.GetDataStart()` directly


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115073/new/

https://reviews.llvm.org/D115073