<div dir="ltr">Yea I don't plan to impl entire classes.  If I end up with any patches that I'm unsure about, I'll upload them for you to look at first.<br></div><br><div class="gmail_quote">On Tue, Mar 3, 2015 at 10:44 AM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is fine as long as you don't take a simple class that contains very few things (like ClangASTType) and try to hide everything behind a unique_ptr just to avoid clang includes. I don't want to trade of compile time for run time performance (heap fragmentation).<br>
<br>
Your fix in this patch was fine as it moved a ClangASTContext into a shared pointer which is fine as it is a large class.<br>
<br>
> On Mar 3, 2015, at 10:37 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> Thanks, I'll take this as a sign that it's ok to commit further patches of this nature.  Let me know if you have concerns about any of the patches I submit.  Removing header file includes from other headers breaks some cpp files, so I'll make sure to test on all 3 platforms before comitting anything.<br>
><br>
><br>
> REPOSITORY<br>
>  rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D8022" target="_blank">http://reviews.llvm.org/D8022</a><br>
><br>
> EMAIL PREFERENCES<br>
>  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/<u></u>settings/panel/<u></u>emailpreferences/</a><br>
><br>
><br>
<br>
</blockquote></div>