remove SkTCast

SkTCast is functionally equivalent to reinterpret_cast.

The comment about SkTCast helping to avoid strict alising issues is not
true.  Dereferencing a pointer cast to a pointer of an unrelated type is
always undefined, even if smuggled through a union like in SkTCast.

To really avoid aliasing issues, you need to make a union[1] of the two
value types, or better, memcpy between values.  I've had to fix
MatrixText.cpp where switching to reinterpret_cast actually let Clang
notice and warn that we're exploiting undefined behavior, and
GrSwizzle.h and SkCamera.cpp caught by GCC.

I've switched SkTLList over to use SkAlignedSTStorage, which seems
to help convince some GCC versions that fObj is used in a sound way.

[1] The union punning trick is non-standard in C++, but GCC and MSVC
both explicitly support it.  I believe Clang does not officially
explicitly support it, but probably does quietly for GCC compatibility.

Change-Id: I71822e82c962f9aaac8be24d3c0f39f4f8b05026
Reviewed-on: https://skia-review.googlesource.com/134947
Commit-Queue: Mike Klein <mtklein@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: Mike Klein <mtklein@chromium.org>
Reviewed-by: Brian Salomon <bsalomon@google.com>

13 files changed, 46 insertions, 50 deletions

diff --git a/bench/MathBench.cpp b/bench/MathBench.cpp
index 7f3408e8e4..74b89a3204 100644
--- a/bench/MathBench.cpp
+++ b/bench/MathBench.cpp
@@ -76,8 +76,8 @@ protected:
                               int count) = 0;
 
     void performTest(float* SK_RESTRICT dst, const float* SK_RESTRICT src, int count) override {
-        uint32_t* d = SkTCast<uint32_t*>(dst);
-        const uint32_t* s = SkTCast<const uint32_t*>(src);
+        uint32_t* d = reinterpret_cast<uint32_t*>(dst);
+        const uint32_t* s = reinterpret_cast<const uint32_t*>(src);
         this->performITest(d, s, count);
     }
 private:
diff --git a/bench/RectBench.cpp b/bench/RectBench.cpp
index 386fb43dfb..407c3a976f 100644
--- a/bench/RectBench.cpp
+++ b/bench/RectBench.cpp
@@ -185,7 +185,7 @@ protected:
             for (size_t i = 0; i < sizes; i++) {
                 paint.setStrokeWidth(gSizes[i]);
                 this->setupPaint(&paint);
-                canvas->drawPoints(fMode, N * 2, SkTCast<SkPoint*>(fRects), paint);
+                canvas->drawPoints(fMode, N * 2, reinterpret_cast<SkPoint*>(fRects), paint);
                 paint.setColor(fColors[i % N]);
             }
         }
@@ -263,7 +263,7 @@ protected:
                 this->setupPaint(&paint);
                 paint.setColor(color);
                 paint.setAlpha(alpha);
-                canvas->drawPoints(fMode, N * 2, SkTCast<SkPoint*>(fRects), paint);
+                canvas->drawPoints(fMode, N * 2, reinterpret_cast<SkPoint*>(fRects), paint);
            }
         }
     }
diff --git a/include/core/SkTLazy.h b/include/core/SkTLazy.h
index a166f05a74..26ee922985 100644
--- a/include/core/SkTLazy.h
+++ b/include/core/SkTLazy.h
@@ -61,7 +61,7 @@ public:
         if (this->isValid()) {
             fPtr->~T();
         }
-        fPtr = new (SkTCast<T*>(fStorage.get())) T(std::forward<Args>(args)...);
+        fPtr = new (reinterpret_cast<T*>(fStorage.get())) T(std::forward<Args>(args)...);
         return fPtr;
     }
 
@@ -75,7 +75,7 @@ public:
         if (this->isValid()) {
             *fPtr = src;
         } else {
-            fPtr = new (SkTCast<T*>(fStorage.get())) T(src);
+            fPtr = new (reinterpret_cast<T*>(fStorage.get())) T(src);
         }
         return fPtr;
     }
@@ -84,7 +84,7 @@ public:
         if (this->isValid()) {
             *fPtr = std::move(src);
         } else {
-            fPtr = new (SkTCast<T*>(fStorage.get())) T(std::move(src));
+            fPtr = new (reinterpret_cast<T*>(fStorage.get())) T(std::move(src));
         }
         return fPtr;
     }
diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h
index 262002bbda..e001bb1068 100644
--- a/include/core/SkTypes.h
+++ b/include/core/SkTypes.h
@@ -247,20 +247,6 @@ enum class SkBackingFit {
     kExact
 };
 
-///////////////////////////////////////////////////////////////////////////////
-
-/**
- *  Use to cast a pointer to a different type, and maintaining strict-aliasing
- */
-template <typename Dst> Dst SkTCast(const void* ptr) {
-    union {
-        const void* src;
-        Dst dst;
-    } data;
-    data.src = ptr;
-    return data.dst;
-}
-
 //////////////////////////////////////////////////////////////////////////////
 
 /** \class SkNoncopyable
diff --git a/include/private/GrSwizzle.h b/include/private/GrSwizzle.h
index db645dbe49..da5d3e0f16 100644
--- a/include/private/GrSwizzle.h
+++ b/include/private/GrSwizzle.h
@@ -38,8 +38,12 @@ private:
         , fKey((CToI(c[0]) << 0) | (CToI(c[1]) << 2) | (CToI(c[2]) << 4) | (CToI(c[3]) << 6)) {}
 
     GR_STATIC_ASSERT(sizeof(char[4]) == sizeof(uint32_t));
-    uint32_t* asUIntPtr() { return SkTCast<uint32_t*>(fSwiz); }
-    uint32_t asUInt() const { return *SkTCast<const uint32_t*>(fSwiz); }
+    uint32_t* asUIntPtr() { return reinterpret_cast<uint32_t*>(fSwiz); }
+    uint32_t asUInt() const {
+        uint32_t v;
+        memcpy(&v, fSwiz, 4);
+        return v;
+    }
 
 public:
     GrSwizzle() { *this = RGBA(); }
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index 2c8eb84845..097569fab2 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -723,11 +723,11 @@ SkDraw::RectType SkDraw::ComputeRectType(const SkPaint& paint,
 }
 
 static const SkPoint* rect_points(const SkRect& r) {
-    return SkTCast<const SkPoint*>(&r);
+    return reinterpret_cast<const SkPoint*>(&r);
 }
 
 static SkPoint* rect_points(SkRect& r) {
-    return SkTCast<SkPoint*>(&r);
+    return reinterpret_cast<SkPoint*>(&r);
 }
 
 static void draw_rect_as_path(const SkDraw& orig, const SkRect& prePaintRect,
diff --git a/src/core/SkScan_AntiPath.cpp b/src/core/SkScan_AntiPath.cpp
index 9c64f32496..13ca3c8b9a 100644
--- a/src/core/SkScan_AntiPath.cpp
+++ b/src/core/SkScan_AntiPath.cpp
@@ -497,7 +497,7 @@ static void add_aa_span(uint8_t* alpha, U8CPU startAlpha, int middleCount,
 
     if (middleCount >= MIN_COUNT_FOR_QUAD_LOOP) {
         // loop until we're quad-byte aligned
-        while (SkTCast<intptr_t>(alpha) & 0x3) {
+        while (reinterpret_cast<intptr_t>(alpha) & 0x3) {
             alpha[0] = SkToU8(alpha[0] + maxValue);
             alpha += 1;
             middleCount -= 1;
diff --git a/src/core/SkTLList.h b/src/core/SkTLList.h
index b865538c27..8680ea2e45 100644
--- a/src/core/SkTLList.h
+++ b/src/core/SkTLList.h
@@ -8,9 +8,9 @@
 #ifndef SkTLList_DEFINED
 #define SkTLList_DEFINED
 
-#include "SkTInternalLList.h"
-
 #include "SkMalloc.h"
+#include "SkTInternalLList.h"
+#include "SkTemplates.h"
 #include "SkTypes.h"
 #include <new>
 #include <utility>
@@ -32,7 +32,7 @@ template <typename T, unsigned int N> class SkTLList : SkNoncopyable {
 private:
     struct Block;
     struct Node {
-        char fObj[sizeof(T)];
+        SkAlignedSTStorage<1, T> fObj;
         SK_DECLARE_INTERNAL_LLIST_INTERFACE(Node);
         Block* fBlock; // owning block.
     };
@@ -51,7 +51,7 @@ public:
         typename NodeList::Iter iter;
         Node* node = iter.init(fList, Iter::kHead_IterStart);
         while (node) {
-            SkTCast<T*>(node->fObj)->~T();
+            reinterpret_cast<T*>(node->fObj.get())->~T();
             Block* block = node->fBlock;
             node = iter.next();
             if (0 == --block->fNodesInUse) {
@@ -71,7 +71,7 @@ public:
         Node* node = this->createNode();
         fList.addToHead(node);
         this->validate();
-        return new (node->fObj)  T(std::forward<Args>(args)...);
+        return new (node->fObj.get())  T(std::forward<Args>(args)...);
     }
 
     /** Adds a new element to the list at the tail. */
@@ -80,7 +80,7 @@ public:
         Node* node = this->createNode();
         fList.addToTail(node);
         this->validate();
-        return new (node->fObj) T(std::forward<Args>(args)...);
+        return new (node->fObj.get()) T(std::forward<Args>(args)...);
     }
 
     /** Adds a new element to the list before the location indicated by the iterator. If the
@@ -90,7 +90,7 @@ public:
         Node* node = this->createNode();
         fList.addBefore(node, location.getNode());
         this->validate();
-        return new (node->fObj) T(std::forward<Args>(args)...);
+        return new (node->fObj.get()) T(std::forward<Args>(args)...);
     }
 
     /** Adds a new element to the list after the location indicated by the iterator. If the
@@ -100,7 +100,7 @@ public:
         Node* node = this->createNode();
         fList.addAfter(node, location.getNode());
         this->validate();
-        return new (node->fObj) T(std::forward<Args>(args)...);
+        return new (node->fObj.get()) T(std::forward<Args>(args)...);
     }
 
     /** Convenience methods for getting an iterator initialized to the head/tail of the list. */
@@ -133,7 +133,7 @@ public:
     void remove(T* t) {
         this->validate();
         Node* node = reinterpret_cast<Node*>(t);
-        SkASSERT(reinterpret_cast<T*>(node->fObj) == t);
+        SkASSERT(reinterpret_cast<T*>(node->fObj.get()) == t);
         this->removeNode(node);
         this->validate();
     }
@@ -210,7 +210,7 @@ public:
 
         T* nodeToObj(Node* node) {
             if (node) {
-                return reinterpret_cast<T*>(node->fObj);
+                return reinterpret_cast<T*>(node->fObj.get());
             } else {
                 return nullptr;
             }
@@ -264,7 +264,7 @@ private:
     void removeNode(Node* node) {
         SkASSERT(node);
         fList.remove(node);
-        SkTCast<T*>(node->fObj)->~T();
+        reinterpret_cast<T*>(node->fObj.get())->~T();
         Block* block = node->fBlock;
         // Don't ever elease the first block, just add its nodes to the free list
         if (0 == --block->fNodesInUse && block != &fFirstBlock) {
diff --git a/src/gpu/GrProgramDesc.cpp b/src/gpu/GrProgramDesc.cpp
index c552789d9f..e5da3be6ee 100644
--- a/src/gpu/GrProgramDesc.cpp
+++ b/src/gpu/GrProgramDesc.cpp
@@ -65,7 +65,7 @@ static void add_sampler_and_image_keys(GrProcessorKeyBuilder* b, const GrResourc
     if (0 == word32Count) {
         return;
     }
-    uint16_t* k16 = SkTCast<uint16_t*>(b->add32n(word32Count));
+    uint16_t* k16 = reinterpret_cast<uint16_t*>(b->add32n(word32Count));
     int j = 0;
     for (int i = 0; i < numTextureSamplers; ++i, ++j) {
         const GrResourceIOProcessor::TextureSampler& sampler = proc.textureSampler(i);
diff --git a/src/utils/SkCamera.cpp b/src/utils/SkCamera.cpp
index cb364a504e..3475bb53d5 100644
--- a/src/utils/SkCamera.cpp
+++ b/src/utils/SkCamera.cpp
@@ -218,13 +218,13 @@ void SkCamera3D::doUpdate() const {
     fAxis.normalize(&axis);
 
     {
-        SkScalar dot = SkUnit3D::Dot(*SkTCast<const SkUnit3D*>(&fZenith), axis);
+        SkScalar dot = SkUnit3D::Dot(SkUnit3D{fZenith.fX, fZenith.fY, fZenith.fZ}, axis);
 
         zenith.fX = fZenith.fX - dot * axis.fX;
         zenith.fY = fZenith.fY - dot * axis.fY;
         zenith.fZ = fZenith.fZ - dot * axis.fZ;
 
-        SkTCast<SkPoint3D*>(&zenith)->normalize(&zenith);
+        SkPoint3D{zenith.fX, zenith.fY, zenith.fZ}.normalize(&zenith);
     }
 
     SkUnit3D::Cross(axis, zenith, &cross);
@@ -276,8 +276,8 @@ void SkCamera3D::patchToMatrix(const SkPatch3D& quilt, SkMatrix* matrix) const {
     diff.fY = quilt.fOrigin.fY - fLocation.fY;
     diff.fZ = quilt.fOrigin.fZ - fLocation.fZ;
 
-    dot = SkUnit3D::Dot(*SkTCast<const SkUnit3D*>(&diff),
-                        *SkTCast<const SkUnit3D*>(SkTCast<const SkScalar*>(&fOrientation) + 6));
+    dot = SkUnit3D::Dot(SkUnit3D{diff.fX,   diff.fY,   diff.fZ},
+                        SkUnit3D{mapPtr[6], mapPtr[7], mapPtr[8]});
 
     // This multiplies fOrientation by the matrix [quilt.fU quilt.fV diff] -- U, V, and diff are
     // column vectors in the matrix -- then divides by the length of the projection of diff onto
diff --git a/tests/MatrixTest.cpp b/tests/MatrixTest.cpp
index 5dfe2ca936..03661dc437 100644
--- a/tests/MatrixTest.cpp
+++ b/tests/MatrixTest.cpp
@@ -27,6 +27,12 @@ static bool nearly_equal(const SkMatrix& a, const SkMatrix& b) {
     return true;
 }
 
+static int float_bits(float f) {
+    int result;
+    memcpy(&result, &f, 4);
+    return result;
+}
+
 static bool are_equal(skiatest::Reporter* reporter,
                       const SkMatrix& a,
                       const SkMatrix& b) {
@@ -38,8 +44,8 @@ static bool are_equal(skiatest::Reporter* reporter,
             for (int i = 0; i < 9; ++i) {
                 float aVal = a.get(i);
                 float bVal = b.get(i);
-                int aValI = *SkTCast<int*>(&aVal);
-                int bValI = *SkTCast<int*>(&bVal);
+                int aValI = float_bits(aVal);
+                int bValI = float_bits(bVal);
                 if (0 == aVal && 0 == bVal && aValI != bValI) {
                     foundZeroSignDiff = true;
                 } else {
@@ -52,8 +58,8 @@ static bool are_equal(skiatest::Reporter* reporter,
             for (int i = 0; i < 9; ++i) {
                 float aVal = a.get(i);
                 float bVal = b.get(i);
-                int aValI = *SkTCast<int*>(&aVal);
-                int bValI = *SkTCast<int*>(&bVal);
+                int aValI = float_bits(aVal);
+                int bValI = float_bits(bVal);
                 if (sk_float_isnan(aVal) && aValI == bValI) {
                     foundNaN = true;
                 } else {
diff --git a/tests/PointTest.cpp b/tests/PointTest.cpp
index 76ac3a1a79..4981e89dde 100644
--- a/tests/PointTest.cpp
+++ b/tests/PointTest.cpp
@@ -14,8 +14,8 @@ static void test_casts(skiatest::Reporter* reporter) {
     SkPoint p = { 0, 0 };
     SkRect  r = { 0, 0, 0, 0 };
 
-    const SkScalar* pPtr = SkTCast<const SkScalar*>(&p);
-    const SkScalar* rPtr = SkTCast<const SkScalar*>(&r);
+    const SkScalar* pPtr = reinterpret_cast<const SkScalar*>(&p);
+    const SkScalar* rPtr = reinterpret_cast<const SkScalar*>(&r);
 
     REPORTER_ASSERT(reporter, SkPointPriv::AsScalars(p) == pPtr);
     REPORTER_ASSERT(reporter, r.asScalars() == rPtr);
diff --git a/tests/RectangleTextureTest.cpp b/tests/RectangleTextureTest.cpp
index 90b723a3eb..e473ed86f3 100644
--- a/tests/RectangleTextureTest.cpp
+++ b/tests/RectangleTextureTest.cpp
@@ -55,7 +55,7 @@ static void test_clear(skiatest::Reporter* reporter, GrSurfaceContext* rectConte
 
         // The clear color is a GrColor, our readback is to kRGBA_8888, which may be different.
         uint32_t expectedColor0 = 0;
-        uint8_t* expectedBytes0 = SkTCast<uint8_t*>(&expectedColor0);
+        uint8_t* expectedBytes0 = reinterpret_cast<uint8_t*>(&expectedColor0);
         expectedBytes0[0] = GrColorUnpackR(color0);
         expectedBytes0[1] = GrColorUnpackG(color0);
         expectedBytes0[2] = GrColorUnpackB(color0);
@@ -70,7 +70,7 @@ static void test_clear(skiatest::Reporter* reporter, GrSurfaceContext* rectConte
         rtc->clear(&rect, color1, GrRenderTargetContext::CanClearFullscreen::kNo);
 
         uint32_t expectedColor1 = 0;
-        uint8_t* expectedBytes1 = SkTCast<uint8_t*>(&expectedColor1);
+        uint8_t* expectedBytes1 = reinterpret_cast<uint8_t*>(&expectedColor1);
         expectedBytes1[0] = GrColorUnpackR(color1);
         expectedBytes1[1] = GrColorUnpackG(color1);
         expectedBytes1[2] = GrColorUnpackB(color1);