[cfe-dev] [PATCH] enable dynamic loading components for static analyzer

[Ted Kremenek]

Looks great.  Please apply.

On Nov 26, 2008, at 5:36 PM, Zhongxing Xu wrote:

> Fixed. New patch attached.
>
> On Thu, Nov 27, 2008 at 9:28 AM, Ted Kremenek <kremenek at apple.com>  
> wrote:
>
> On Nov 26, 2008, at 5:14 PM, Zhongxing Xu wrote:
>
> +/// ManagerRegister - This class records manager creators  
> registered at
> +/// runtime. The information is communicated to AnalysisManager  
> through static
> +/// members. Better design is expected.
> +
> +class ManagerRegister {
> +public:
> +  typedef ConstraintManager* (*ConstraintManagerCreator) 
> (GRStateManager&);
> +  typedef StoreManager* (*StoreManagerCreator)(GRStateManager&);
>
> 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.
>
> Done. But this introduced two #includes, commented in code. And I  
> noticed there are other #includes only for including typedefs, for  
> example LiveSymbolsTy/DeadSymbolsTy.
>
> I named the class ManagerRegistry instead of ManagerRegistrar.
>
> Other comments were adopted. Thanks.
>
> New patch attached.
> <load2.patch>
>
>
> Thanks Zhongxing.  A few comments:
>
> ===================================================================
> --- include/clang/Analysis/PathSensitive/GRState.h      (revision  
> 60131)
> +++ include/clang/Analysis/PathSensitive/GRState.h      (working copy)
> @@ -27,6 +27,7 @@
>  #include "clang/AST/Decl.h"
>  #include "clang/AST/ASTContext.h"
>  #include "clang/Analysis/Analyses/LiveVariables.h"
> +#include "clang/Driver/ManagerRegistry.h" // for typedefs.
>
> 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).
>
>
> Index: include/clang/Analysis/PathSensitive/GRExprEngine.h
> ===================================================================
> --- include/clang/Analysis/PathSensitive/GRExprEngine.h (revision  
> 60131)
>
> +++ include/clang/Analysis/PathSensitive/GRExprEngine.h (working copy)
> @@ -22,6 +22,7 @@
>  #include "clang/Analysis/PathSensitive/GRTransferFuncs.h"
>  #include "clang/AST/Type.h"
>  #include "clang/AST/ExprObjC.h"
> +#include "clang/Driver/ManagerRegistry.h" // for typedefs.
>
> This is the same layering violation.  Have the typedefs be in  
> libAnalysis, not libDriver.
>
>
> <load3.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081126/dfaa2cd1/attachment.html>