[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...

[Pavel Labath via lldb-commits]

On 11 August 2016 at 00:41, Zachary Turner via lldb-commits
<lldb-commits at lists.llvm.org> wrote:
> 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.

+1
Also note the presence of the ASSERT_XXX macros. The work the same as
the EXPECT_ versions, but also abort the current test (more precisely,
they just issue a "return", as gtest does not rely on exceptions).
That way you can do ASSERT_NE(nullptr, foo_sp), and in the rest of the
function you can assume that the check was successful.