[Lldb-commits] [lldb] r278304 - Fix a problem where if a uint64_t value is placed into a python dictionary and sent up to LLDB and converted to StructuredData, it would not be able to parse the full 64 bit value. A number like 0xf000000000000000L could be placed into a dictionary, and sent to LLDB and it would end up being 0xffffffffffffffff since it would overflow a int64_t. We leave the old code there, but if it overflows, we treat the number like a uint64_t and get it to decode correctly. Added a gtest to cover this...

[Zachary Turner via lldb-commits]

Couple of random style comments below.  You don't have to fix these, just
something to think about in the future.



On Wed, Aug 10, 2016 at 4:33 PM Greg Clayton via lldb-commits <
lldb-commits at lists.llvm.org> wrote:

> +
> +TEST_F(PythonDataObjectsTest, TestExtractingUInt64ThroughStructuredData)
> +{
> +    // Make up a custom dictionary with "sys" pointing to the `sys`
> module.
> +    const char *key_name = "addr";
> +    const uint64_t value = 0xf000000000000000ull;
> +    PythonDictionary python_dict(PyInitialValue::Empty);
> +    PythonInteger python_ull_value(PyRefType::Owned,
> PyLong_FromUnsignedLongLong(value));
> +    python_dict.SetItemForKey(PythonString(key_name), python_ull_value);
> +    StructuredData::ObjectSP structured_data_sp =
> python_dict.CreateStructuredObject();
> +    EXPECT_TRUE((bool)structured_data_sp);
>
EXPECT_NE(nullptr, structured_data_sp);

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.


> +    if (structured_data_sp)
> +    {
> +        StructuredData::Dictionary *structured_dict_ptr =
> structured_data_sp->GetAsDictionary();
> +        EXPECT_TRUE(structured_dict_ptr != nullptr);
> +        if (structured_dict_ptr)
> +        {
> +            StructuredData::ObjectSP structured_addr_value_sp =
> structured_dict_ptr->GetValueForKey(key_name);
> +            EXPECT_TRUE((bool)structured_addr_value_sp);
> +            const uint64_t extracted_value =
> structured_addr_value_sp->GetIntegerValue(123);
> +            EXPECT_TRUE(extracted_value == value);
>
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.


> +        }
> +    }
> +}
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160810/88fddbf6/attachment.html>