[llvm] r346429 - [NativePDB] Higher fidelity reconstruction of AST from Debug Info.

[David Blaikie via llvm-commits]

On Mon, Nov 12, 2018 at 11:42 AM Zachary Turner <zturner at google.com> wrote:

> The tests in llvm just get the whole flags enum and then format it. So if
> this flag is set in the output, you’ll see it. But it doesn’t seem easy to
> make that formatting function go through this method. That said, I think
> the important thing to test is that this flag properly gets set in the enum
> in the correct condition. So the current test tests that I think. But this
> helper method seems obviously correct (and pretty trivial) and not worth
> adding a special test for
>

I tend to be a bit overly test-enthusiastic, admittedly (& my favourite
example of a trivial function with a bug & no test case is still r188582) -
so I'll hardly argue to the death about it - but I'd prefer code is tested,
even if it's pretty simple stuff like this.


> On Mon, Nov 12, 2018 at 11:36 AM David Blaikie <dblaikie at gmail.com> wrote:
>
>> Maybe we do have coverage but not through this member function - if the
>> code for nested classes that is exercised by LLVM lit or unit tests
>> (assuming it's tested somewhere) could be changed to use this member
>> function, then it'd be tested & I'd be good with it.
>>
>> Yeah, I don't mind what form the tests take - but just asking the general
>> "this is a change to LLVM, does it/can it/should it have a test in LLVM?"
>>
>> On Mon, Nov 12, 2018 at 11:10 AM Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> I believe we do already have test coverage for nested classes, i was
>>> just adding this helper method so I didn’t have to write the long if
>>> statement. But if we don’t have coverage for nested types, then maybe a lit
>>> test would be better?
>>> On Mon, Nov 12, 2018 at 11:04 AM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> This is, 'technically', a separate patch in some sense from the LLD
>>>> part - changes to LLVM usually merit tests in LLVM to ensure they
>>>> work/don't regress/etc without needing to checkout & test other subprojects
>>>> (like lld).
>>>>
>>>> On Mon, Nov 12, 2018 at 11:02 AM Zachary Turner <zturner at google.com>
>>>> wrote:
>>>>
>>>>> This one specific function, or the patch as a whole? I thought the
>>>>> patch had pretty good non-unit-test coverage
>>>>> On Mon, Nov 12, 2018 at 10:36 AM David Blaikie <dblaikie at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Could this use/be tested with a unit test, perhaps?
>>>>>>
>>>>>> On Thu, Nov 8, 2018 at 10:52 AM Zachary Turner via llvm-commits <
>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Author: zturner
>>>>>>> Date: Thu Nov  8 10:50:11 2018
>>>>>>> New Revision: 346429
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=346429&view=rev
>>>>>>> Log:
>>>>>>> [NativePDB] Higher fidelity reconstruction of AST from Debug Info.
>>>>>>>
>>>>>>> In order to accurately put a type into the correct location in the
>>>>>>> AST
>>>>>>> we construct from debug info, we need to be able to determine what
>>>>>>> DeclContext (namespace, global, nested class, etc) that it goes into.
>>>>>>> PDB doesn't contain this mapping.  It does, however, contain the
>>>>>>> reverse
>>>>>>> mapping.  That is, for a given class type T, you can determine all
>>>>>>> classes Q1, Q2, ..., Qn that are nested inside of T.  We need to
>>>>>>> know,
>>>>>>> for a given class type Q, what type T is it nested inside of.
>>>>>>>
>>>>>>> This patch builds this map as a pre-processing step when we first
>>>>>>> load the PDB by scanning every type.  Initial tests show that while
>>>>>>> this can be slow in debug builds of LLDB, it is quite fast in release
>>>>>>> builds (less than 2 seconds for a ~1GB PDB, and it only needs to
>>>>>>> happen
>>>>>>> once).
>>>>>>>
>>>>>>> Furthermore, having this pre-processing step in place allows us to
>>>>>>> repurpose it for building up other kinds of indexing to it down the
>>>>>>> line.  For the time being, this gives us very accurate reconstruction
>>>>>>> of the DeclContext hierarchy.
>>>>>>>
>>>>>>> Differential Revision: https://reviews.llvm.org/D54216
>>>>>>>
>>>>>>> Modified:
>>>>>>>     llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h
>>>>>>>
>>>>>>> Modified: llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h?rev=346429&r1=346428&r2=346429&view=diff
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h
>>>>>>> (original)
>>>>>>> +++ llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h Thu Nov
>>>>>>> 8 10:50:11 2018
>>>>>>> @@ -429,6 +429,10 @@ public:
>>>>>>>      return (Options & ClassOptions::ForwardReference) !=
>>>>>>> ClassOptions::None;
>>>>>>>    }
>>>>>>>
>>>>>>> +  bool containsNestedClass() const {
>>>>>>> +    return (Options & ClassOptions::ContainsNestedClass) !=
>>>>>>> ClassOptions::None;
>>>>>>> +  }
>>>>>>> +
>>>>>>>    bool isScoped() const {
>>>>>>>      return (Options & ClassOptions::Scoped) != ClassOptions::None;
>>>>>>>    }
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181112/be381848/attachment.html>