Ok, I think I worked through the issues. Please take another look :D
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:54-56
@@ -53,1 +53,5 @@
+REGISTER_TRAIT_WITH_PROGRAMSTATE(
+ InitializedTemporariesSet,
+ llvm::ImmutableSet<const CXXBindTemporaryExpr *>);
+
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Jordan Rose wrote:
> > > This needs to include the current StackFrameContext as well (for
> > > recursive functions). You can get that from the current LocationContext.
> > Can you elaborate on how I would put that into a datastructure? Just use a
> > std::pair? (doesn't seem to work with ImmutableSet though)
> >
> > Also, I seem unable to write a test that breaks because of this - any hints
> > would be highly welcome.
> We'd have to write a specialization of ImutProfileInfo for std::pair, but
> that's probably not a bad idea anyway. Please feel free to add one to
> ImmutableSet.h.
>
> I would guess the test case would look something like this:
>
> static bool test(bool isInner) {
> if (isInner || NoReturnDtor() || test(true)) {
> *(volatile int *)0 = 1; // should warn
> }
> }
> void topLevel() {
> test(false);
> }
>
Sent patch for review & added test.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+ !State->contains<InitializedTemporariesSet>(BTE));
+ State = State->add<InitializedTemporariesSet>(BTE);
+ StmtBldr.generateNode(BTE, Pred, State);
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Alex McCarthy wrote:
> > > If includeTemporaryDtorsInCFG is not set, should we avoid this state
> > > modification?
> > That's a good question (for Jordan ;)
> >
> > Another question is whether we still want to do the runCheckersForPre /
> > runCheckersForPost enchilada.
> We're trying to run pre/post-checks on all non-trivial statements, so yes, we
> should still do it. It's //almost// simple enough that we can hoist that out
> of the big switch, but I'm not sure that's correct for all statement kinds,
> and no one's looked into it.
>
> (But now that we have C++11, I'm //really// inclined to hoist the thing into
> a closure, so that people don't have to write that "loop over the nodes
> generated by pre-checkers" loop themselves.)
>
> As for the state modification, what I mainly care about is that we //clean
> up// the map. Since that doesn't happen when temporary destructors are off,
> yes, we should be checking here. (Note that this is also important if we stop
> generating conditions for every temporary destructor—we need to make sure
> that the state is clean by the end of the full-expression even if we skip
> some of the conditions.)
Done.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
Jordan Rose wrote:
> Manuel Klimek wrote:
> > Alex McCarthy wrote:
> > > I think it's worth pulling this out into a new processTemporary helper
> > > method rather than adding it to processBranch: you're not reusing any of
> > > the logic in processBranch in the temporary dtor case.
> > >
> > > This would involve adding a new HandleTemporary helper (or similar) that
> > > you'd call instead of HandleBranch, and that would call processTemporary
> > I agree. I mainly haven't done that, as somewhere in that chain there is an
> > interface, and the method is abstract. I found only one use, so I assume
> > somebody else implements that interface outside of the clang codebase. I'd
> > like to hear what Jordan thinks on that issue, architecture-wise.
> These days the interface isn't serving us much real purpose. In theory
> CoreEngine is just traversing CFGs, and doesn't actually know very much about
> C ef al. It then hands the CFGElements off to its SubEngine, which actually
> processes them and tells the CoreEngine where to go next if there's a branch.
> ExprEngine is the particularly SubEngine that understands the AST / the
> language.
>
> In practice there's a lot of linguistic knowledge coded into CoreEngine, just
> because there's a lot of linguistic knowledge in the CFG and in ProgramPoints
> these days. But either way, SubEngine is more of an interface than a real
> base class; I don't think Ted ever expected that we'd get subclassing more
> than one level deep. It's okay to split this up into helper methods.
Done.
http://reviews.llvm.org/D3627
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
