Swizzler changes Index8 and 565

include/core/SkTemplates.h

 
 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkTemplates_DEFINED
@@ skipping 94 matching lines @@
 */
 template <typename T, int (*P)(T*)> class SkAutoTCallIProc : SkNoncopyable {
 public:
     SkAutoTCallIProc(T* obj): fObj(obj) {}
     ~SkAutoTCallIProc() { if (fObj) P(fObj); }
 
     operator T*() const { return fObj; }
     T* operator->() const { SkASSERT(fObj); return fObj; }
 
     T* detach() { T* obj = fObj; fObj = NULL; return obj; }
+    void reset(T* obj = NULL) {
+        if (fObj != obj) {
+            if (fObj) {
+                P(fObj);
+            }
+            fObj = obj;
+        }
+    }

[msarett, 2015/04/09 16:26:32]

I'm sure it is possible to implement gif rewinding without this API, but it
seemed to me that having this API was the most straightforward way to do it.

Also, it seems natural to have this API available.  An identical function exists
for SkAutoTCallVProc.  Why not for SkAutoTCallIProc?

I am a little unsure if maybe we would want it to return an int though?

[scroggo, 2015/04/09 17:37:17]

git blame is your friend. Using blame, I see we added SkAutoTCallVProc::reset in
https://codereview.chromium.org/396143004, when it was needed.

So it was probably just an oversight because we didn't need it for
SkAutoTCallIProc. I think adding this is reasonable.
That seems reasonable to me, since P returns an int. One thing I am unsure
about, though, is what to return in the following cases:

fObj == obj
!fObj

Both of these are *not* failure cases, but we will not have called P(fObj). We
do not know what return values of P are meaningful, so how can we choose what to
return?

This would be an argument against returning an int (maybe with a comment
explaining that). An alternative (ugly) approach would be to use out
parameter(s, or one out parameter + return value) to specify whether we called P
and if so, what the return value is.

I think a cleaner approach is to decide that if a client wants to know the
return value of P, they have to do the following:

T* obj = autoProc.detach();
int val = P(obj);
// Do something with val
autoProc.reset(newObj);

You'll need API approval for such a change, so you can follow up with reed@.

Looking back at your callsite, though, you do not ever care about the return
value of P(), and it's a function you wrote (SkGifCodec::CloseGif). I think you
should change it to return void, and then you can call SkAutoTCallVProc, which
already has reset().

[msarett, 2015/04/09 19:21:31]

All good points.  Seems like using SkAutoTCallCProc is a no brainer.  We don't
ever need the return value.

 private:
     T* fObj;
 };
 
 /** \class SkAutoTDelete
   An SkAutoTDelete<T> is like a T*, except that the destructor of SkAutoTDelete<T>
   automatically deletes the pointer it holds (if any).  That is, SkAutoTDelete<T>
   owns the T object that it points to.  Like a T*, an SkAutoTDelete<T> may hold
   either NULL or a pointer to a T object.  Also like T*, SkAutoTDelete<T> is
   thread-compatible, and once you dereference it, you get the threadsafety
@@ skipping 382 matching lines @@
     /**
      * Returns void* because this object does not initialize the
      * memory. Use placement new for types that require a cons.
      */
     void* get() { return fStorage.get(); }
 private:
     SkAlignedSStorage<sizeof(T)*N> fStorage;
 };
 
 #endif