[Debug Info PATCH] patches to generate unique identifier for external C++ types

[Eric Christopher]

On Thu, Aug 29, 2013 at 3:36 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Thu, Aug 29, 2013 at 2:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Please include test case changes along with whichever patch requires
>> them, generally that's a bit easier to keep track of rather than
>> having to pair a given change with a given patch.
>>
>> The Clang change could use the code review feedback I gave earlier
>> today - have the unique string function return a SmallString & rely on
>> NRVO, rather than passing by reference.
>
> It seems that the other parts of the compiler pass a SmallString & and
> return void, so I am not sure
> whether we should depend on NRVO :)
>

Personal opinion: if it makes the code more legible then always depend
on NRVO. If it doesn't fire and it's a hot spot in the code then we
can worry about it then :)

-eric

>>
>> (also, you have a FIXME in
>> there about objective C++ - is there any reason you aren't just
>> checking both language values now? Or checking the language setting in
>> LangOpts (in there, CPlusPlus is true for any C++ variant, including
>> Objective C++) - but I'm happy to leave it out if you've got concerns
>> about Objective-C types when used in Objective C++, I know nothing
>> about such things)
>
> I want to separate C++ from Objective C++, I was thinking about enabling
> Objective C++ in a later patch.
>
>>
>>
>> The comment in DIBuilder::finalize might use some more detail. Two
>> different nodes will be added to the retained types list, but the
>> client (Clang in this instance) deduplicates those after the fact,
>> thus leaving the retained types list with duplicate nodes. Perhaps:
>>
>> // Declarations and definitions of the same type may be retained. Some
>> clients RAUW these pairs, leaving duplicates in the retained types
>> list. Use a set to remove the duplicates while we transform the
>> TrackingVHs back into Values.
>
> Will do.
>
> Thanks again for the quick review,
> Manman
>>
>>
>> With those changes, please go ahead & commit these patches.
>>
>> On Thu, Aug 29, 2013 at 2:33 PM, Manman Ren <manman.ren at gmail.com> wrote:
>> > Hi,
>> >
>> > I think it is a good idea to start a new thread :)
>> > Attached please find 3 patches: one for llvm (update DIBuilder to retain
>> > types with unique
>> > identifier), one for clang (to generate the unique identifier), the last
>> > for
>> > updating testing cases.
>> >
>> > Thanks,
>> > Manman
>
>