Fixed. New patch attached.<br><br><div class="gmail_quote">On Thu, Nov 27, 2008 at 9:28 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c"><br>
On Nov 26, 2008, at 5:14 PM, Zhongxing Xu wrote:<br>
<br>
</div></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="Wj3C7c">
+/// ManagerRegister - This class records manager creators registered at<br>
+/// runtime. The information is communicated to AnalysisManager through static<br>
+/// members. Better design is expected.<br>
+<br>
+class ManagerRegister {<br>
+public:<br>
+  typedef ConstraintManager* (*ConstraintManagerCreator)(GRStateManager&);<br>
+  typedef StoreManager* (*StoreManagerCreator)(GRStateManager&);<br>
<br>
Aren't these typedefs the same as in the ones in GRStateManager?  Maybe we should just move those typedefs into the clang namespace and have them used by both GRStateManager and ManagerRegister (if not, let's just keep the ones in GRStateManager).  It seems silly to have the same typedefs defined twice, which can lead to weird warnings if we don't keep them in sync.<br>

<br>
Done. But this introduced two #includes, commented in code. And I noticed there are other #includes only for including typedefs, for example LiveSymbolsTy/DeadSymbolsTy.<br>
<br>
I named the class ManagerRegistry instead of ManagerRegistrar.<br>
<br>
Other comments were adopted. Thanks.<br>
<br>
New patch attached.<br></div></div>
<load2.patch><br>
</blockquote>
<br>
<br>
Thanks Zhongxing.  A few comments:<br>
<br>
===================================================================<br>
--- include/clang/Analysis/PathSensitive/GRState.h      (revision 60131)<br>
+++ include/clang/Analysis/PathSensitive/GRState.h      (working copy)<br>
@@ -27,6 +27,7 @@<br>
 #include "clang/AST/Decl.h"<br>
 #include "clang/AST/ASTContext.h"<br>
 #include "clang/Analysis/Analyses/LiveVariables.h"<br>
+#include "clang/Driver/ManagerRegistry.h" // for typedefs.<br>
<br>
This is a layering violation.  libAnalysis should not depend on libDriver, it should be the other way.  That's the whole point of putting ManagerRegistry in libDriver.  The typedefs belong in libAnalysis, either in GRState.h or in a new header file (one that is small).<div class="Ih2E3d">
<br>
<br>
Index: include/clang/Analysis/PathSensitive/GRExprEngine.h<br>
===================================================================<br></div>
--- include/clang/Analysis/PathSensitive/GRExprEngine.h (revision 60131)<div class="Ih2E3d"><br>
+++ include/clang/Analysis/PathSensitive/GRExprEngine.h (working copy)<br></div>
@@ -22,6 +22,7 @@<br>
 #include "clang/Analysis/PathSensitive/GRTransferFuncs.h"<br>
 #include "clang/AST/Type.h"<br>
 #include "clang/AST/ExprObjC.h"<br>
+#include "clang/Driver/ManagerRegistry.h" // for typedefs.<br>
<br>
This is the same layering violation.  Have the typedefs be in libAnalysis, not libDriver.<br>
<br>
</blockquote></div><br>