Make fID and MixedID calculations private

src/core/SkGlyph.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 SkGlyph_DEFINED
 #define SkGlyph_DEFINED
 
 #include "SkTypes.h"
 #include "SkFixed.h"
 #include "SkMask.h"
 
 class SkPath;
+class SkGlyphCache;
 
 // needs to be != to any valid SkMask::Format
 #define MASK_FORMAT_UNKNOWN         (0xFF)
 #define MASK_FORMAT_JUST_ADVANCE    MASK_FORMAT_UNKNOWN
 
 #define kMaxGlyphWidth (1<<13)
 
-struct SkGlyph {
+class SkGlyph {
+    enum {
+        kSubBits = 2,
+        kSubMask = ((1 << kSubBits) - 1),
+        kSubShift = 24, // must be large enough for glyphs and unichars
+        kCodeMask = ((1 << kSubShift) - 1),
+        // relative offsets for X and Y subpixel bits
+            kSubShiftX = kSubBits,

[mtklein, 2015/02/23 17:30:36]

funky indent?

[herb_g, 2015/02/23 23:24:53]

Done.

+        kSubShiftY = 0
+    };
+
+ public:
+    static const SkFixed kSubpixelRound = SK_FixedHalf >> SkGlyph::kSubBits;
     void*       fImage;
     SkPath*     fPath;
     SkFixed     fAdvanceX, fAdvanceY;
 
-    uint32_t    fID;
     uint16_t    fWidth, fHeight;
     int16_t     fTop, fLeft;
 
     uint8_t     fMaskFormat;
     int8_t      fRsbDelta, fLsbDelta;  // used by auto-kerning
     int8_t      fForceBW;
 
-    void init(uint32_t id) {
-        fID             = id;
-        fImage          = NULL;
-        fPath           = NULL;
-        fMaskFormat     = MASK_FORMAT_UNKNOWN;
-        fForceBW        = 0;
+    void initWithGlyphID(uint32_t glyph_id) {
+        this->initCommon(MakeID(glyph_id));
+    }
+
+    void initWithGlyphIDXY(uint32_t glyph_id, SkFixed x, SkFixed y) {

[mtklein, 2015/02/23 17:30:36]

Huh, unused.  Must be only SkGlyphCache does this?  Let's leave it out if we're
going to keep SkGlyphCache as a superfriend.

[herb_g, 2015/02/23 23:24:53]

Done.

+        this->initCommon(MakeID(glyph_id, x, y));
+    }
+
+    void initFromGlyph(const SkGlyph& glyph) {

[mtklein, 2015/02/23 17:30:36]

maybe copyFrom() ?

[herb_g, 2015/02/23 23:24:53]

This is not really copying the whole structure, but only the glyph id. It's
really an init. Tried a better name.

+        this->initCommon(glyph.fID);
     }
 
     /**
      *  Compute the rowbytes for the specified width and mask-format.
      */
     static unsigned ComputeRowBytes(unsigned width, SkMask::Format format) {
         unsigned rb = width;
         if (SkMask::kBW_Format == format) {
             rb = (rb + 7) >> 3;
         } else if (SkMask::kARGB32_Format == format) {
@@ skipping 35 matching lines @@
     }
 
     size_t computeImageSize() const;
 
     /** Call this to set all of the metrics fields to 0 (e.g. if the scaler
         encounters an error measuring a glyph). Note: this does not alter the
         fImage, fPath, fID, fMaskFormat fields.
      */
     void zeroMetrics();
 
-    enum {
-        kSubBits = 2,
-        kSubMask = ((1 << kSubBits) - 1),
-        kSubShift = 24, // must be large enough for glyphs and unichars
-        kCodeMask = ((1 << kSubShift) - 1),
-        // relative offsets for X and Y subpixel bits
-        kSubShiftX = kSubBits,
-        kSubShiftY = 0
-    };
 
+    void toMask(SkMask* mask) const;
+
+ private:
+    // TODO(herb) remove friend statement after SkGlyphCache cleanup.
+    friend class SkGlyphCache;
+
+    void initCommon(uint32_t id) {
+        fID             = id;
+        fImage          = NULL;
+        fPath           = NULL;
+        fMaskFormat     = MASK_FORMAT_UNKNOWN;
+        fForceBW        = 0;
+    }
     static unsigned ID2Code(uint32_t id) {
-        return id & kCodeMask;
+        return (id & kCodeMask) - 1;

[mtklein, 2015/02/23 17:30:36]

Now that it's clear this is safe, let's undo this for this CL.  It's weird to
land it without the extra context of why we're trying to carve zero out as an
invalid value.  Really, when SkGlyphCache is no longer a friend, we'll probably
want an SkGlyph::initInvalid() to hide away how we do that too.  It may be
clearer, for example, to use ~0 instead of 0 as the invalid value.

[herb_g, 2015/02/23 23:24:53]

If you use ~0, then one record will always be at the end of the array, so you
will always be adjust an extra record. With sentinal == 0 then you will never
move the extra record.

     }
 
     static unsigned ID2SubX(uint32_t id) {
         return id >> (kSubShift + kSubShiftX);
     }
 
     static unsigned ID2SubY(uint32_t id) {
         return (id >> (kSubShift + kSubShiftY)) & kSubMask;
     }
 
     static unsigned FixedToSub(SkFixed n) {
         return (n >> (16 - kSubBits)) & kSubMask;
     }
 
     static SkFixed SubToFixed(unsigned sub) {
         SkASSERT(sub <= kSubMask);
         return sub << (16 - kSubBits);
     }
 
     static uint32_t MakeID(unsigned code) {
-        return code;
+        return code + 1;
     }
 
     static uint32_t MakeID(unsigned code, SkFixed x, SkFixed y) {
         SkASSERT(code <= kCodeMask);
         x = FixedToSub(x);
         y = FixedToSub(y);
         return (x << (kSubShift + kSubShiftX)) |
-               (y << (kSubShift + kSubShiftY)) |
-               code;
+            (y << (kSubShift + kSubShiftY)) |
+            (code + 1);
     }
 
-    void toMask(SkMask* mask) const;
+    uint32_t    fID;
 };
 
 #endif