[cfe-commits] r169563 - in /cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive: CallEvent.h CheckerContext.h ExplodedGraph.h

[Jordan Rose]

On Dec 6, 2012, at 16:07 , David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Dec 6, 2012 at 3:55 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> Author: jrose
>> Date: Thu Dec  6 17:55:34 2012
>> New Revision: 169563
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=169563&view=rev
>> Log:
>> [analyzer] Avoid ProgramStateRef copy constructors.
>> 
>> Suggested by David Blaikie. ExplodedNode, CallEvent, and CheckerContext all
>> hang onto their ProgramState, so the accessors can return a reference to the
>> internal state rather than preemptively copying it. This helps avoid
>> temporary ProgramStateRefs, though local variables will still (correctly)
>> do an extra retain and release.
>> 
>> Modified:
>>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
>>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
>>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
>> 
>> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h?rev=169563&r1=169562&r2=169563&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h (original)
>> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h Thu Dec  6 17:55:34 2012
>> @@ -142,10 +142,12 @@
>> protected:
>>   friend class CallEventManager;
>> 
>> -  CallEvent(const Expr *E, ProgramStateRef state, const LocationContext *lctx)
>> +  CallEvent(const Expr *E, const ProgramStateRef &state,
>> +            const LocationContext *lctx)
>>     : State(state), LCtx(lctx), Origin(E), RefCount(0) {}
> 
> FWIW - when passing in to take ownership (which wasn't the case in the
> particular call chain I was looking at in your last review - that one
> just called getPtr()) you could prefer to pass by value and use swap
> to transfer ownership. This will avoid a retain/release in the case of
> passing in a temporary.
> 
> (passing things by value and using swap (or move assignment in C++11)
> to take ownership is a nice habit to get into as it becomes more
> common/powerful/useful in C++11 with movable types being a common
> situation (whereas these days optimizing for swappable types in this
> way is a bit less common))
> 
> Just a thought, in case this is important to you/this code.
> 
> - David

IntrusiveRefCntPtr is already properly move-assignable. These methods don't acquire ownership in general, though...there really are cases where multiple CallEvents or ExplodedNodes may refer to the same ProgramState. There are a few cases, though, where the local variables that feed into these things aren't being used afterwards (or if they come from temporaries).

We probably would see a win just compiling as C++11. Thanks!

Jordan