[Lldb-commits] [PATCH] D20565: Add MemoryRegionInfo to SB API

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So a bit more cleanup. We should always have a valid object inside of a SBMemoryRegionInfo or SBMemoryRegionInfoList so there is no need to ever check the m_opaque_ap to see if it contains a valid pointer so all if statements that are doing:

  if (m_opaque_ap.get())
      m_opaque_ap->DoSomething();

Can be just:

  m_opaque_ap->DoSomething();

Also, no functions should ever call "m_opaque_ap->reset();". All the changes should be easy after that.


================
Comment at: source/API/SBMemoryRegionInfo.cpp:45-48
@@ +44,6 @@
+    {
+        if (rhs.IsValid())
+            ref() = rhs.ref();
+        else
+            m_opaque_ap.reset();
+    }
----------------
These 4 lines should just be:

```
ref() = rhs.ref();
```

We never want m_opaque_ap to contain an invalid pointer or we can crash.

================
Comment at: source/API/SBMemoryRegionInfo.cpp:60
@@ +59,3 @@
+{
+    return m_opaque_ap.get() != NULL;
+}
----------------
We shouldn't check for NULL since the auto pointer should never be invalid. This should just call through and ask the underlying object:

```
return m_opaque_ap->IsValid();
```

If there is no IsValid() function on the underlying object, this function can be removed.

================
Comment at: source/API/SBMemoryRegionInfo.cpp:66
@@ +65,3 @@
+{
+    m_opaque_ap.reset();
+}
----------------
We should never clear the underlying object, this should just call through:

```
m_opaque_ap->Clear();
```

================
Comment at: source/API/SBMemoryRegionInfo.cpp:72-74
@@ +71,5 @@
+{
+    if (m_opaque_ap)
+        return m_opaque_ap.get() == rhs.m_opaque_ap.get();
+    return false;
+}
----------------
This should compare the objects:

```
return ref() == rhs.ref();
```

================
Comment at: source/API/SBMemoryRegionInfo.cpp:80-82
@@ +79,5 @@
+{
+    if (m_opaque_ap)
+        return m_opaque_ap.get() != rhs.m_opaque_ap.get();
+    return false;
+}
----------------
This should compare the objects:

```
return ref() != rhs.ref();
```

================
Comment at: source/API/SBMemoryRegionInfo.cpp:88-89
@@ +87,4 @@
+{
+    if (m_opaque_ap.get() == NULL)
+        m_opaque_ap.reset (new MemoryRegionInfo ());
+    return *m_opaque_ap;
----------------
You should remove these lines now since m_opaque_ap will never contain an invalid object.

================
Comment at: source/API/SBMemoryRegionInfo.cpp:101-104
@@ +100,6 @@
+SBMemoryRegionInfo::GetRegionBase () {
+    if (m_opaque_ap.get())
+    {
+        return m_opaque_ap->GetRange().GetRangeBase();
+    }
+    return 0;
----------------
No need to check the object

================
Comment at: source/API/SBMemoryRegionInfo.cpp:110-113
@@ +109,6 @@
+SBMemoryRegionInfo::GetRegionEnd () {
+    if (m_opaque_ap.get())
+    {
+        return m_opaque_ap->GetRange().GetRangeEnd();
+    }
+    return 0;
----------------
No need to check the object, just call the accessor

================
Comment at: source/API/SBMemoryRegionInfoList.cpp:108
@@ +107,3 @@
+{
+    if (m_opaque_ap.get())
+        return m_opaque_ap->GetSize();
----------------
No need to check the object, just call the accessor. Remove the if and the other return.

================
Comment at: source/API/SBMemoryRegionInfoList.cpp:136
@@ +135,3 @@
+{
+    if (m_opaque_ap.get())
+        m_opaque_ap->Clear();
----------------
No need to check the object, just call the accessor. Remove the if statement.

================
Comment at: source/API/SBMemoryRegionInfoList.cpp:143
@@ +142,3 @@
+{
+    if (sb_region.IsValid() && m_opaque_ap.get())
+        m_opaque_ap->Append(sb_region);
----------------
No need to check m_opaque_ap since it will always be valid.

================
Comment at: source/API/SBMemoryRegionInfoList.cpp:150
@@ +149,3 @@
+{
+    if (sb_region_list.IsValid() && m_opaque_ap.get())
+        m_opaque_ap->Append(*sb_region_list);
----------------
No need to check m_opaque_ap since it will always be valid.

================
Comment at: source/API/SBMemoryRegionInfoList.cpp:157
@@ +156,3 @@
+{
+    return m_opaque_ap.get() != NULL;
+}
----------------
No need for an IsValid() since the list is always valid, this function can actually be removed. 


http://reviews.llvm.org/D20565