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

[Zhongxing Xu]

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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081127/c7e1837a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: load3.patch
Type: application/octet-stream
Size: 7911 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081127/c7e1837a/attachment.obj>