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

[Reid Kleckner via llvm-commits]

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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160603/e7f6e835/attachment.html>