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

[David Blaikie]

On Thu, Dec 6, 2012 at 5:13 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> 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.

Sorry, I misspoke indeed. I just meant "making a copy of the smart
pointer" (acquire an owning pointer?). The essential semantic,
regardless of the specifics of this case: if you're always going to
make a copy of parameter, accept the parameter by value and make a
copy with swap or move assignment.

>  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!

The trick is you lose in the temporary case when you use this approach
compared to the accept-by-value-and-swap case. What you have now
(accepting by const ref and copying) will copy even in the case where
you pass in a temporary. If you accept by value then copy elision (or
move semantics) can kick in and the copy in the caller can be elided,
then the swap/move assignment occurs and there's no ref count changes.

By no means necessary, but if you're looking to get this kind of level
of perf out of the code, that's how you might go about it & it's not
terribly obfuscatory.

- David