SkValue: SkXfermode

src/core/SkValue.cpp

+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include <unordered_map>
+
+#include "SkValue.h"
+#include "SkData.h"
+#include "SkRefCnt.h"
+
+class SkValue::Obj : public SkRefCnt {
+public:
+    std::unordered_map<SkValue::Key, SkValue> fMap;
+};
+
+SkValue::SkValue() : fType(SkValue::Null) {}
+
+SkValue::SkValue(const SkValue& o) {
+    memcpy(this, &o, sizeof(o));
+    if (SkValue::Bytes == fType) {
+        reinterpret_cast<SkData*>(fBytes)->ref();

[mtklein, 2016/01/15 00:25:22]

WTF?  If you want to have an SkData* member, just change it to be an SkData*.

[hal.canary, 2016/01/15 13:38:56]

I started by trying to implelment this without changing your header, just to
test my other code.

done.

+    } else if (fType > 0xFF) {
+        fObject->ref();

[mtklein, 2016/01/15 00:25:22]

This makes no sense.  You can't share anything that's mutable.  If you change
one object you'll change the other.

[hal.canary, 2016/01/15 13:38:56]

If I add a flag that gets set on the first copy that makes the entire thing
immutable, will that work?

[mtklein, 2016/01/15 13:46:56]

Yes, but that's an extremely fragile design.  If you want to do that, pass
ownership of the map from a MutableObject to a ConstObject... change its type. 
Then you'll need to copy (really copy) it back to a MutableObject on the next
write, etc.  It also all needs to be thread safe.

So don't do any of that.  Copy.

+    }
+}
+
+SkValue::SkValue(SkValue&& o) {
+    memcpy(this, &o, sizeof(o));
+    new(&o) SkValue();

[mtklein, 2016/01/15 00:25:22]

space after new?

[hal.canary, 2016/01/15 13:38:56]

done

+}
+
+SkValue& SkValue::operator=(const SkValue& o) {
+    if (this != &o) {
+        this->~SkValue();
+        new (this) SkValue(o);
+    }
+    return *this;
+}
+SkValue& SkValue::operator=(SkValue&& o) {
+    if (this != &o) {
+        this->~SkValue();
+        new (this) SkValue(std::move(o));
+    }
+    return *this;
+}
+
+SkValue::~SkValue() {
+    if (SkValue::Bytes == fType) {
+        reinterpret_cast<SkData*>(fBytes)->unref();
+    } else if (fType > 0xFF) {

[mtklein, 2016/01/15 00:25:22]

Let's add

private: 
    bool isObject() const { fType > 0xFF }

[hal.canary, 2016/01/15 13:38:56]

done.

+        fObject->unref();
+    }
+}
+
+SkValue SkValue::FromS32(int32_t x) {
+    SkValue v;
+    v.fType = SkValue::S32;

[mtklein, 2016/01/15 00:25:22]

in this scope you can just write S32, etc.

[hal.canary, 2016/01/15 13:38:56]

done.

+    v.fS32 = x;
+    return v;
+}
+
+SkValue SkValue::FromU32(uint32_t x) {
+    SkValue v;
+    v.fType = SkValue::U32;
+    v.fU32 = x;
+    return v;
+}
+
+SkValue SkValue::FromF32(float x) {
+    SkValue v;
+    v.fType = SkValue::F32;
+    v.fF32 = x;
+    return v;
+}
+
+SkValue SkValue::FromBytes(const void* data, size_t length) {
+    SkValue v;
+    v.fType = SkValue::Bytes;
+    reinterpret_cast<SkData*&>(v.fBytes) = SkData::NewWithCopy(data, length);
+    return v;
+}
+
+SkValue SkValue::Object(SkValue::Type t) {
+    SkASSERT(t > 0xFF);
+    SkValue v;
+    v.fType = t;
+    v.fObject = new SkValue::Obj;
+    return v;
+}
+
+SkValue::Type SkValue::type() const { return fType; }
+
+int32_t  SkValue::s32() const { return SkValue::S32 == fType ? fS32 : 0; }

[mtklein, 2016/01/15 00:25:22]

These should all assert they're the right type.

[hal.canary, 2016/01/15 13:38:56]

done.

+
+uint32_t SkValue::u32() const { return SkValue::U32 == fType ? fU32 : 0; }
+
+float    SkValue::f32() const { return SkValue::F32 == fType ? fF32 : 0; }
+
+const void* SkValue::bytes() const {
+    return SkValue::Bytes == fType
+        ? reinterpret_cast<SkData*>(fBytes)->data()
+        : nullptr;
+}
+
+size_t SkValue::count() const {
+    if (SkValue::Bytes == fType) {
+        return reinterpret_cast<SkData*>(fBytes)->size();
+    } else if (fType > 0xFF) {
+        return fObject->fMap.size();

[mtklein, 2016/01/15 00:25:22]

let's just have count() work for Bytes for now.

[hal.canary, 2016/01/15 13:38:56]

why?

[mtklein, 2016/01/15 13:46:56]

We have foreach().

+    }
+    return 0;
+}
+
+void SkValue::set(SkValue::Key k, SkValue v) {
+    if (fType <= 0xFF) {
+        return;
+    }
+    fObject->fMap[k] = std::move(v);
+}
+const SkValue* SkValue::get(SkValue::Key k) const {
+    if (fType <= 0xFF) {
+        return nullptr;
+    }
+    auto& map = fObject->fMap;
+    return map.find(k) != map.end() ? &map[k] : nullptr;

[mtklein, 2016/01/15 00:25:22]

This does the hash lookup twice.

const auto& map = fObject->fMap;
for (auto it = map.find(k); it != map.end(); ) {
    return &*it;
}
return nullptr;

[hal.canary, 2016/01/15 13:38:56]

done.  reading the specs it looked like the iterator returned a copy.  but
testing shows that it doesn't.

[mtklein, 2016/01/15 13:46:56]

That map iterator is a little surprising, I admit, especially on a team that
doesn't have experience using the STL.

+}
+void SkValue::foreach(
+        std::function<void(SkValue::Key, const SkValue&)> fn) const {
+    if (fType <= 0xFF) {
+        return;
+    }
+    for (const auto& pair : fObject->fMap) {
+        fn(pair.first, pair.second);
+    }
+}