<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 29, 2013 at 2:48 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Please include test case changes along with whichever patch requires<br>

them, generally that's a bit easier to keep track of rather than<br>
having to pair a given change with a given patch.<br>
<br>
The Clang change could use the code review feedback I gave earlier<br>
today - have the unique string function return a SmallString & rely on<br>
NRVO, rather than passing by reference. </blockquote><div>It seems that the other parts of the compiler pass a SmallString & and return void, so I am not sure<br></div><div>whether we should depend on NRVO :)</div><div>
 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">(also, you have a FIXME in<br>
there about objective C++ - is there any reason you aren't just<br>
checking both language values now? Or checking the language setting in<br>
LangOpts (in there, CPlusPlus is true for any C++ variant, including<br>
Objective C++) - but I'm happy to leave it out if you've got concerns<br>
about Objective-C types when used in Objective C++, I know nothing<br>
about such things)<br></blockquote><div>I want to separate C++ from Objective C++, I was thinking about enabling</div><div>Objective C++ in a later patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<br>
The comment in DIBuilder::finalize might use some more detail. Two<br>
different nodes will be added to the retained types list, but the<br>
client (Clang in this instance) deduplicates those after the fact,<br>
thus leaving the retained types list with duplicate nodes. Perhaps:<br>
<br>
// Declarations and definitions of the same type may be retained. Some<br>
clients RAUW these pairs, leaving duplicates in the retained types<br>
list. Use a set to remove the duplicates while we transform the<br>
TrackingVHs back into Values.<br></blockquote><div>Will do.</div><div><br></div><div>Thanks again for the quick review,</div><div>Manman </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<br>
With those changes, please go ahead & commit these patches.<br>
<div class=""><div class="h5"><br>
On Thu, Aug 29, 2013 at 2:33 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
> Hi,<br>
><br>
> I think it is a good idea to start a new thread :)<br>
> Attached please find 3 patches: one for llvm (update DIBuilder to retain<br>
> types with unique<br>
> identifier), one for clang (to generate the unique identifier), the last for<br>
> updating testing cases.<br>
><br>
> Thanks,<br>
> Manman<br>
</div></div></blockquote></div><br></div></div>