<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 7, 2015 at 8:38 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There's a few minor cosmetic issues that apply across multiple files:<br>
<br>
- Some if-statements have extra spaces inside the parentheses. "if (foo)" not "if ( foo )".<br>
- Some files are inconsistent about * and & placement in types. We usually place them next to the variable names.<br>
- include/clang/Sema/ActiveTemplateInst.h has extra blank lines.<br>
<br>
You may find it simplest to use clang-format-diff.py.<br>
<br>
I think we'll want a way to test out this interface; it should probably come with a command-line tool that has a very simple template observer that prints out the callbacks as they're called, and a test which runs this tool to exercise it. This might also be a FrontendAction in clang, similar to -ast-dump, instead of a separate tool. (This is not a trivial amount of work, you may want to wait for another clang developer to chime in before spending time on it.)<br></blockquote><div><br></div><div>Other than -ast-dump, there is also pp-trace which is in clang-tools-extra. However, we don't depend on pp-trace for the actual testing. It seems we test PPCallbacks with a unittest.</div><div><br></div><div>Personally, I would prefer something FileCheck-able though.</div><div><br></div><div><br></div><div>(also, minor bikeshed, but naming this *Callbacks instead of *Observer to match with PPCallbacks would be nice)</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Overall I'm happy with the design!<br>
<br>
<br>
================<br>
Comment at: include/clang/Sema/TemplateInstObserver.h:25-26<br>
@@ +24,4 @@<br>
+<br>
+  class Sema;<br>
+  class ActiveTemplateInstantiation;<br>
+<br>
----------------<br>
Alphabetize.<br>
<br>
================<br>
Comment at: include/clang/Sema/TemplateInstObserver.h:28<br>
@@ +27,3 @@<br>
+<br>
+/// \brief This is a base class for an observer that will be notified (callback) at every template instantiation.<br>
+class TemplateInstantiationObserver {<br>
----------------<br>
What about a callback when clang transitions from parsing the TU to actOnEndOfTranslationUnit? Template instantiation is different before and after we reach the end-of-TU.<br>
<br>
================<br>
Comment at: lib/Sema/SemaType.cpp:5202<br>
@@ -5200,1 +5201,3 @@<br>
     }<br>
+    else if (Def && TemplateInstObserverChain) {<br>
+      ActiveTemplateInstantiation TempInst;<br>
----------------<br>
Is there a guarantee that this is a template? Offhand it looks like this could fire on any struct, couldn't it?<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="http://reviews.llvm.org/D5767" target="_blank">http://reviews.llvm.org/D5767</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>