[llvm] r271408 - [codeview] Translate basic DITypes to CV type records

[David Blaikie via llvm-commits]

On Fri, Jun 3, 2016 at 8:43 AM, Reid Kleckner <rnk at google.com> wrote:

> On Wed, Jun 1, 2016 at 10:22 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> +
>>> +  // Check if we've already translated this type.
>>> +  auto I = TypeIndices.find(Ty);
>>> +  if (I != TypeIndices.end())
>>> +    return I->second;
>>> +
>>>
>>
>> Might be worth adding a comment somewhere here to mention that lowerType
>> might add new TypeIndices and thus you can't avoid the double lookup (find
>> and then unhinted insert) here. Pretty obvious I guess, but seems nice to
>> have.
>>
>
> I added this comment in a later patch, and Amjad found it confusing:
> http://reviews.llvm.org/D20924#inline-177103
>

Right - as you said there, I think that's just a matter of finding the
right wording/place to put it.


>
>
> At least for the DWARF tests we dwarfdump the output of LLVM and check
>> against the pretty printed dump output. Is there a reason we need/prefer to
>> test the asm directly here?
>>
>
> inlining.ll already tests both the assembly output and the readobj output.
> The readobj output is a lot easier to read and validate for correctness,
> but I think it's also worth having a few tests of the assembly output,
> especially since inlining uses a bunch of custom directives.
>

But this was a change for type info, right? Why would that have an impact
on inlining? Does the inlined debug info with its custom directives
reference the type hierarchy? (this isn't true in DWARF, where the inline
info isn't part of the line table and the line table is very 'raw' - but I
could imagine formats where it is true)

Does readobj do pretty dumping the same way pdbdump does? Seems a bit
asymmetric with dwarfdump - should we roll dwarf dumping into readobj &
kill dwarfdump for consistency? Or pull out CV record dumping? Or just is
insufficiently analogous?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160603/a263f83b/attachment.html>