<div dir="ltr">Couple of random style comments below.  You don't have to fix these, just something to think about in the future.<div><br></div><div><br></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 10, 2016 at 4:33 PM Greg Clayton via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org">lldb-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+<br>
+TEST_F(PythonDataObjectsTest, TestExtractingUInt64ThroughStructuredData)<br>
+{<br>
+    // Make up a custom dictionary with "sys" pointing to the `sys` module.<br>
+    const char *key_name = "addr";<br>
+    const uint64_t value = 0xf000000000000000ull;<br>
+    PythonDictionary python_dict(PyInitialValue::Empty);<br>
+    PythonInteger python_ull_value(PyRefType::Owned, PyLong_FromUnsignedLongLong(value));<br>
+    python_dict.SetItemForKey(PythonString(key_name), python_ull_value);<br>
+    StructuredData::ObjectSP structured_data_sp = python_dict.CreateStructuredObject();<br>
+    EXPECT_TRUE((bool)structured_data_sp);<br></blockquote><div>EXPECT_NE(nullptr, structured_data_sp);</div><div><br></div><div>would be preferable here.  If EXPECT_TRUE fails, you will just an error message saying it was false when it should have been true.  If EXPECT_NE fails, you will get an error message telling you the expected value and the actual value.  This is most useful when the actual and expected values are integers or strings, but the same concept applies anywhere.  Using the proper EXPECT macro gives you a better error message.  Same goes in a few other places below.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    if (structured_data_sp)<br>
+    {<br>
+        StructuredData::Dictionary *structured_dict_ptr = structured_data_sp->GetAsDictionary();<br>
+        EXPECT_TRUE(structured_dict_ptr != nullptr);<br>
+        if (structured_dict_ptr)<br>
+        {<br>
+            StructuredData::ObjectSP structured_addr_value_sp = structured_dict_ptr->GetValueForKey(key_name);<br>
+            EXPECT_TRUE((bool)structured_addr_value_sp);<br>
+            const uint64_t extracted_value = structured_addr_value_sp->GetIntegerValue(123);<br>
+            EXPECT_TRUE(extracted_value == value);<br></blockquote><div>Here's an example of where EXPECT_EQ(value, extracted_value) would be really helpful.  Without it, you'd need to debug the test to to see what the actual value is.  With it, it will print both values so you can often easily determine the cause of the failure without any additional effort.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+        }<br>
+    }<br>
+}<br></blockquote><div><span style="line-height:1.5">As a general rule of thumb, we should probably avoid conditionals in unit tests unless they're really necessary.  It's easy to end up in cases where your test ends up not testing something because it's behind a conditional.  EXPECT_NE(nullptr, foo_sp) is probably fine, then just assume it's non null.</span><br></div></div></div>