[WIP] Add Context to SkDrawLooper.

src/core/SkTemplatesPriv.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 SkTemplatesPriv_DEFINED
 #define SkTemplatesPriv_DEFINED
 
 #include "SkTemplates.h"
 
 ////////////////////////////////////////////////////////////////////////////////
 
 #ifdef SK_BUILD_FOR_WIN32

[bungeman-skia, 2014/02/11 17:06:21]

This seems to be an anachronism. I see no indication that vc++ ever had any
actual issues with placement new (and if it does I'm fairly sure there are other
places in Skia which would break). Note that SkPostConfig.h already provides
SkNEW_PLACEMENT which is used elsewhere on all platforms. I just disabled this
(#if 0) and it builds fine on vs2012 (and I'm sure on all others). It looks like
this is mostly holdover from the very early attempt at a Windows port since this
file and content appear to come from before Windows was actually supported.

     #define SK_PLACEMENT_NEW(result, classname, storage, storageSize)   \
         result = SkNEW(classname)
 
     #define SK_PLACEMENT_NEW_ARGS(result, classname, storage, storageSize, args)    \
         result = SkNEW_ARGS(classname, args)
+
+    #define SK_PLACEMENT_SAFE_NEW(result, classname, storage, storageSize)   \
+        result = SkNEW(classname)

[scroggo, 2014/02/11 15:51:29]

Yikes! This will result in a memory leak if the caller is not careful. Would it
make sense to have some kind of SK_PLACEMENT_SAFE_DELETE that handles the delete
the way you do your delete?

I see this is already what we do on WIN32 for normal placement new (which I
cannot imagine the caller accounting for). Does placement new not work on WIN32?

(The history of SK_PLACEMENT_NEW on WIN32 is lost to time - it was pulled from
Android in 2008.)

[Dominik Grewe, 2014/02/11 16:04:31]

I guess the same is true for the normal placement method. No matter which one is
used, you always have to be careful about freeing memory.

Having SK_PLACEMENT_DELETE would make sense; I don't think we need a special
SK_PLACEMENT_SAFE_DELETE but we could add it for "symmetry".
Tbh, I'm not sure why we need this special case for WIN32, I just assumed we do
because that's what we do the normal placement functions. I can have a look if
it's still needed.

[scroggo, 2014/02/11 16:14:26]

Well, this one is actually safer than the normal placement method. When calling
this, you (should) know that you have to special case the delete, but if someone
called SK_PLACEMENT_NEW I would expect them to call ~T() instead of delete,
which would be the appropriate place everywhere except WIN32, where we'll leak
the memory. (Sorry to derail your CL a little bit - this problem wasn't
introduced by you!)

On another note, what about using SkAutoSMalloc (in SkTypes.h)? This class
handles choosing whether to use malloc and free (although you then still have to
do the SK_PLACEMENT_NEW yourself).

+
+    #define SK_PLACEMENT_SAFE_NEW_ARGS(result, classname, storage, storageSize, args)    \
+        result = SkNEW_ARGS(classname, args)
 #else
     #include <new>
     #define SK_PLACEMENT_NEW(result, classname, storage, storagesize)       \

[bungeman-skia, 2014/02/11 17:06:21]

Every current user of this does the same dance where they have to do a check to
see what result was and either delete or destroy. The name is also entirely
misleading, as this obviously isn't interchangeable with SkNEW_PLACEMENT. In
other words, the name is terrible and doesn't do what it says it does. Let's not
perpetuate that.

     do {                                                                    \
         if (storagesize)                                                    \
         {                                                                   \
             SkASSERT(storageSize >= sizeof(classname));                     \
             result = new(storage) classname;                                \
         }                                                                   \
         else                                                                \
             result = SkNEW(classname);                                      \
     } while (0)
 
     #define SK_PLACEMENT_NEW_ARGS(result, classname, storage, storagesize, args)        \
     do {                                                                                \
         if (storagesize)                                                                \
         {                                                                               \
             SkASSERT(storageSize >= sizeof(classname));                                 \
             result = new(storage) classname args;                                       \
         }                                                                               \
         else                                                                            \
             result = SkNEW_ARGS(classname, args);                                       \
     } while (0)
+
+    #define SK_PLACEMENT_SAFE_NEW(result, classname, storage, storagesize)  \

[bungeman-skia, 2014/02/11 17:06:21]

Why are we multiplying entities without necessity here? Why the new name? Why
not just replace the content of the SK_PLACEMENT_NEW with these new bodies?
Comparing against the constant sizeof or against 0 here seems a rather small
difference in performance, if that's what we're worrying about.

+    do {                                                                    \
+        if (storagesize >= sizeof(classname))                               \
+            result = new(storage) classname;                                \
+        else                                                                \
+            result = SkNEW(classname);                                      \
+    } while (0)
+
+    #define SK_PLACEMENT_SAFE_NEW_ARGS(result, classname, storage, storagesize, args)   \
+    do {                                                                                \
+        if (storagesize >= sizeof(classname))                                           \
+            result = new(storage) classname args;                                       \
+        else                                                                            \
+            result = SkNEW_ARGS(classname, args);                                       \
+    } while (0)
 #endif
 
 ////////////////////////////////////////////////////////////////////////////////
 
 template <class T> class SkAutoTPlacementDelete {
 public:
     SkAutoTPlacementDelete(T* obj, void* storage) : fObj(obj), fStorage(storage)
     {
     }
     ~SkAutoTPlacementDelete()
@@ skipping 11 matching lines @@
         T*  obj = fObj;
         fObj = NULL;
         return obj;
     }
 private:
     T*      fObj;
     void*   fStorage;
 };
 
 #endif