From 89c909efee2ca2ef5f1eb7e1a2de2126cc0e4433 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Mon, 1 Jun 2020 15:50:19 -0500 Subject: [PATCH] Wsign-conversion for public headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an Android request for our public headers, much like warning about unused parameters. See bug. In general I've made two kinds of source changes: 1) more commonly, explicitly cast to the type which is being implicitly cast to at head; 2) less commonly, flip signedness of a value we're storing to match how it's used more smoothly. Much of this is self inflicted inconsistent use of size_t, unsigned, int, int32_t, uint32_t, etc. SkTArray is particularly tricky because of its std::vector half-compatibility. E.g. resize() takes size_t, but operator[] takes int. ¯\_(ツ)_/¯ Bug: skia:9847 Change-Id: I64626a529e1662b3d3020bc03d477fc641eda544 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/293436 Reviewed-by: Mike Reed Commit-Queue: Mike Klein --- gn/BUILD.gn | 9 ++++++++- include/core/SkFontStyle.h | 2 +- include/core/SkImageInfo.h | 8 +++++--- include/core/SkMatrix.h | 14 +++++++------- include/core/SkPixmap.h | 8 ++++---- include/core/SkTypes.h | 2 +- include/gpu/vk/GrVkTypes.h | 4 ++-- include/private/GrResourceKey.h | 10 +++++----- include/private/SkFixed.h | 8 ++++---- include/private/SkImageInfoPriv.h | 4 ++-- include/private/SkTArray.h | 6 +++--- include/private/SkTDArray.h | 2 +- include/utils/SkRandom.h | 2 +- src/gpu/vk/GrVkSamplerYcbcrConversion.h | 4 ++-- 14 files changed, 46 insertions(+), 37 deletions(-) diff --git a/gn/BUILD.gn b/gn/BUILD.gn index 99435bd776..f1ca64868a 100644 --- a/gn/BUILD.gn +++ b/gn/BUILD.gn @@ -430,6 +430,10 @@ config("warnings") { "-Wno-weak-vtables", ] + # Turn back on after -Wno-conversion. + # This only affects public headers... see :warnings_except_public_headers. + cflags += [ "-Wsign-conversion" ] + # We are unlikely to want to fix these. cflags += [ "-Wno-covered-switch-default", @@ -483,7 +487,10 @@ config("warnings") { } config("warnings_except_public_headers") { if (!is_win || is_clang) { - cflags = [ "-Wno-unused-parameter" ] + cflags = [ + "-Wno-sign-conversion", + "-Wno-unused-parameter", + ] } } diff --git a/include/core/SkFontStyle.h b/include/core/SkFontStyle.h index 50b5bd026d..dfb2ec9dc4 100644 --- a/include/core/SkFontStyle.h +++ b/include/core/SkFontStyle.h @@ -74,7 +74,7 @@ public: } private: - uint32_t fValue; + int32_t fValue; }; #endif diff --git a/include/core/SkImageInfo.h b/include/core/SkImageInfo.h index eacd94d478..a8378c47c5 100644 --- a/include/core/SkImageInfo.h +++ b/include/core/SkImageInfo.h @@ -581,20 +581,22 @@ public: @return width() times bytesPerPixel() as unsigned 64-bit integer */ - uint64_t minRowBytes64() const { return sk_64_mul(this->width(), this->bytesPerPixel()); } + uint64_t minRowBytes64() const { + return (uint64_t)sk_64_mul(this->width(), this->bytesPerPixel()); + } /** Returns minimum bytes per row, computed from pixel width() and SkColorType, which specifies bytesPerPixel(). SkBitmap maximum value for row bytes must fit in 31 bits. - @return width() times bytesPerPixel() as signed 32-bit integer + @return width() times bytesPerPixel() as size_t */ size_t minRowBytes() const { uint64_t minRowBytes = this->minRowBytes64(); if (!SkTFitsIn(minRowBytes)) { return 0; } - return SkTo(minRowBytes); + return (size_t)minRowBytes; } /** Returns byte offset of pixel from pixel base address. diff --git a/include/core/SkMatrix.h b/include/core/SkMatrix.h index 2d669462cd..524070a742 100644 --- a/include/core/SkMatrix.h +++ b/include/core/SkMatrix.h @@ -1717,7 +1717,7 @@ public: fMat[kMPersp1] = 0; fMat[kMPersp2] = 1; - unsigned mask = 0; + int mask = 0; if (sx != 1 || sy != 1) { mask |= kScale_Mask; } @@ -1761,12 +1761,12 @@ private: kPerspective_Mask | kRectStaysRect_Mask; - SkScalar fMat[9]; - mutable uint32_t fTypeMask; + SkScalar fMat[9]; + mutable int32_t fTypeMask; constexpr SkMatrix(SkScalar sx, SkScalar kx, SkScalar tx, SkScalar ky, SkScalar sy, SkScalar ty, - SkScalar p0, SkScalar p1, SkScalar p2, uint32_t typeMask) + SkScalar p0, SkScalar p1, SkScalar p2, int typeMask) : fMat{sx, kx, tx, ky, sy, ty, p0, p1, p2} @@ -1782,18 +1782,18 @@ private: SkASSERT(kUnknown_Mask == mask || (mask & kAllMasks) == mask || ((kUnknown_Mask | kOnlyPerspectiveValid_Mask) & mask) == (kUnknown_Mask | kOnlyPerspectiveValid_Mask)); - fTypeMask = SkToU8(mask); + fTypeMask = mask; } void orTypeMask(int mask) { SkASSERT((mask & kORableMasks) == mask); - fTypeMask = SkToU8(fTypeMask | mask); + fTypeMask |= mask; } void clearTypeMask(int mask) { // only allow a valid mask SkASSERT((mask & kAllMasks) == mask); - fTypeMask = fTypeMask & ~mask; + fTypeMask &= ~mask; } TypeMask getPerspectiveTypeMaskOnly() const { diff --git a/include/core/SkPixmap.h b/include/core/SkPixmap.h index d7beb29568..8cacfd07df 100644 --- a/include/core/SkPixmap.h +++ b/include/core/SkPixmap.h @@ -369,7 +369,7 @@ public: const uint8_t* addr8(int x, int y) const { SkASSERT((unsigned)x < (unsigned)fInfo.width()); SkASSERT((unsigned)y < (unsigned)fInfo.height()); - return (const uint8_t*)((const char*)this->addr8() + y * fRowBytes + (x << 0)); + return (const uint8_t*)((const char*)this->addr8() + (size_t)y * fRowBytes + (x << 0)); } /** Returns readable pixel address at (x, y). @@ -387,7 +387,7 @@ public: const uint16_t* addr16(int x, int y) const { SkASSERT((unsigned)x < (unsigned)fInfo.width()); SkASSERT((unsigned)y < (unsigned)fInfo.height()); - return (const uint16_t*)((const char*)this->addr16() + y * fRowBytes + (x << 1)); + return (const uint16_t*)((const char*)this->addr16() + (size_t)y * fRowBytes + (x << 1)); } /** Returns readable pixel address at (x, y). @@ -405,7 +405,7 @@ public: const uint32_t* addr32(int x, int y) const { SkASSERT((unsigned)x < (unsigned)fInfo.width()); SkASSERT((unsigned)y < (unsigned)fInfo.height()); - return (const uint32_t*)((const char*)this->addr32() + y * fRowBytes + (x << 2)); + return (const uint32_t*)((const char*)this->addr32() + (size_t)y * fRowBytes + (x << 2)); } /** Returns readable pixel address at (x, y). @@ -423,7 +423,7 @@ public: const uint64_t* addr64(int x, int y) const { SkASSERT((unsigned)x < (unsigned)fInfo.width()); SkASSERT((unsigned)y < (unsigned)fInfo.height()); - return (const uint64_t*)((const char*)this->addr64() + y * fRowBytes + (x << 3)); + return (const uint64_t*)((const char*)this->addr64() + (size_t)y * fRowBytes + (x << 3)); } /** Returns readable pixel address at (x, y). diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h index 0701014521..5ab70cd7f6 100644 --- a/include/core/SkTypes.h +++ b/include/core/SkTypes.h @@ -542,7 +542,7 @@ template static constexpr bool SkIsAlignPtr(T x) { typedef uint32_t SkFourByteTag; static inline constexpr SkFourByteTag SkSetFourByteTag(char a, char b, char c, char d) { - return (((uint8_t)a << 24) | ((uint8_t)b << 16) | ((uint8_t)c << 8) | (uint8_t)d); + return (((uint32_t)a << 24) | ((uint32_t)b << 16) | ((uint32_t)c << 8) | (uint32_t)d); } //////////////////////////////////////////////////////////////////////////////// diff --git a/include/gpu/vk/GrVkTypes.h b/include/gpu/vk/GrVkTypes.h index e5d91a43aa..784930b603 100644 --- a/include/gpu/vk/GrVkTypes.h +++ b/include/gpu/vk/GrVkTypes.h @@ -105,7 +105,7 @@ struct GrVkYcbcrConversionInfo { VkChromaLocation yChromaOffset, VkFilter chromaFilter, VkBool32 forceExplicitReconstruction, - uint64_t externalFormat, + int64_t externalFormat, VkFormatFeatureFlags externalFormatFeatures) : GrVkYcbcrConversionInfo(VK_FORMAT_UNDEFINED, externalFormat, ycbcrModel, ycbcrRange, xChromaOffset, yChromaOffset, chromaFilter, @@ -135,7 +135,7 @@ struct GrVkYcbcrConversionInfo { // The external format. Must be non-zero for external images, zero otherwise. // Should be compatible to be used in a VkExternalFormatANDROID struct. - uint64_t fExternalFormat; + int64_t fExternalFormat; VkSamplerYcbcrModelConversion fYcbcrModel; VkSamplerYcbcrRange fYcbcrRange; diff --git a/include/private/GrResourceKey.h b/include/private/GrResourceKey.h index 977eecf25e..ee39e90304 100644 --- a/include/private/GrResourceKey.h +++ b/include/private/GrResourceKey.h @@ -62,7 +62,7 @@ protected: } else { size_t bytes = that.size(); SkASSERT(SkIsAlign4(bytes)); - fKey.reset(SkToInt(bytes / sizeof(uint32_t))); + fKey.reset(bytes / sizeof(uint32_t)); memcpy(fKey.get(), that.fKey.get(), bytes); this->validate(); } @@ -103,10 +103,10 @@ protected: class Builder { public: Builder(GrResourceKey* key, uint32_t domain, int data32Count) : fKey(key) { - SkASSERT(data32Count >= 0); + size_t count = SkToSizeT(data32Count); SkASSERT(domain != kInvalidDomain); - key->fKey.reset(kMetaDataCnt + data32Count); - int size = (data32Count + kMetaDataCnt) * sizeof(uint32_t); + key->fKey.reset(kMetaDataCnt + count); + size_t size = (count + kMetaDataCnt) * sizeof(uint32_t); SkASSERT(SkToU16(size) == size); SkASSERT(SkToU16(domain) == domain); key->fKey[kDomainAndSize_MetaDataIdx] = domain | (size << 16); @@ -128,7 +128,7 @@ protected: SkASSERT(fKey); SkDEBUGCODE(size_t dataCount = fKey->internalSize() / sizeof(uint32_t) - kMetaDataCnt;) SkASSERT(SkToU32(dataIdx) < dataCount); - return fKey->fKey[kMetaDataCnt + dataIdx]; + return fKey->fKey[(int)kMetaDataCnt + dataIdx]; } private: diff --git a/include/private/SkFixed.h b/include/private/SkFixed.h index af858efc89..6e8990f574 100644 --- a/include/private/SkFixed.h +++ b/include/private/SkFixed.h @@ -62,7 +62,7 @@ typedef int32_t SkFixed; SkASSERT(n >= -32768 && n <= 32767); // Left shifting a negative value has undefined behavior in C, so we cast to unsigned before // shifting. - return (unsigned)n << 16; + return (SkFixed)( (unsigned)n << 16 ); } #else // Left shifting a negative value has undefined behavior in C, so we cast to unsigned before @@ -76,13 +76,13 @@ typedef int32_t SkFixed; #define SkFixedFloorToInt(x) ((x) >> 16) static inline SkFixed SkFixedRoundToFixed(SkFixed x) { - return (x + SK_FixedHalf) & 0xFFFF0000; + return (SkFixed)( (uint32_t)(x + SK_FixedHalf) & 0xFFFF0000 ); } static inline SkFixed SkFixedCeilToFixed(SkFixed x) { - return (x + SK_Fixed1 - 1) & 0xFFFF0000; + return (SkFixed)( (uint32_t)(x + SK_Fixed1 - 1) & 0xFFFF0000 ); } static inline SkFixed SkFixedFloorToFixed(SkFixed x) { - return x & 0xFFFF0000; + return (SkFixed)( (uint32_t)x & 0xFFFF0000 ); } #define SkFixedAbs(x) SkAbs32(x) diff --git a/include/private/SkImageInfoPriv.h b/include/private/SkImageInfoPriv.h index 582867162f..6a3c0d1736 100644 --- a/include/private/SkImageInfoPriv.h +++ b/include/private/SkImageInfoPriv.h @@ -74,7 +74,7 @@ static int SkColorTypeShiftPerPixel(SkColorType ct) { } static inline size_t SkColorTypeMinRowBytes(SkColorType ct, int width) { - return width * SkColorTypeBytesPerPixel(ct); + return (size_t)(width * SkColorTypeBytesPerPixel(ct)); } static inline bool SkColorTypeIsValid(unsigned value) { @@ -85,7 +85,7 @@ static inline size_t SkColorTypeComputeOffset(SkColorType ct, int x, int y, size if (kUnknown_SkColorType == ct) { return 0; } - return y * rowBytes + (x << SkColorTypeShiftPerPixel(ct)); + return (size_t)y * rowBytes + ((size_t)x << SkColorTypeShiftPerPixel(ct)); } static inline bool SkColorTypeIsNormalized(SkColorType ct) { diff --git a/include/private/SkTArray.h b/include/private/SkTArray.h index 4334f7da69..4a87e11dd4 100644 --- a/include/private/SkTArray.h +++ b/include/private/SkTArray.h @@ -473,7 +473,7 @@ private: fReserved = false; } else { fAllocCount = std::max(count, std::max(kMinHeapAllocCount, reserveCount)); - fItemArray = (T*)sk_malloc_throw(fAllocCount, sizeof(T)); + fItemArray = (T*)sk_malloc_throw((size_t)fAllocCount, sizeof(T)); fOwnMemory = true; fReserved = reserveCount > 0; } @@ -523,7 +523,7 @@ private: } template std::enable_if_t move(void* dst) { for (int i = 0; i < fCount; ++i) { - new (static_cast(dst) + sizeof(T) * i) T(std::move(fItemArray[i])); + new (static_cast(dst) + sizeof(T) * (size_t)i) T(std::move(fItemArray[i])); fItemArray[i].~T(); } } @@ -569,7 +569,7 @@ private: fAllocCount = Sk64_pin_to_s32(newAllocCount); SkASSERT(fAllocCount >= newCount); - T* newItemArray = (T*)sk_malloc_throw(fAllocCount, sizeof(T)); + T* newItemArray = (T*)sk_malloc_throw((size_t)fAllocCount, sizeof(T)); this->move(newItemArray); if (fOwnMemory) { sk_free(fItemArray); diff --git a/include/private/SkTDArray.h b/include/private/SkTDArray.h index 2cf31625ba..9d475f6f8e 100644 --- a/include/private/SkTDArray.h +++ b/include/private/SkTDArray.h @@ -372,7 +372,7 @@ private: SkASSERT_RELEASE( SkTFitsIn(reserve) ); fReserve = SkTo(reserve); - fArray = (T*)sk_realloc_throw(fArray, fReserve * sizeof(T)); + fArray = (T*)sk_realloc_throw(fArray, (size_t)fReserve * sizeof(T)); } }; diff --git a/include/utils/SkRandom.h b/include/utils/SkRandom.h index 0678010362..ba40732b9c 100644 --- a/include/utils/SkRandom.h +++ b/include/utils/SkRandom.h @@ -49,7 +49,7 @@ public: * Returns value [0...1) as an IEEE float */ float nextF() { - unsigned int floatint = 0x3f800000 | (this->nextU() >> 9); + int floatint = 0x3f800000 | (int)(this->nextU() >> 9); float f = SkBits2Float(floatint) - 1.0f; return f; } diff --git a/src/gpu/vk/GrVkSamplerYcbcrConversion.h b/src/gpu/vk/GrVkSamplerYcbcrConversion.h index 05c10b848b..829129841f 100644 --- a/src/gpu/vk/GrVkSamplerYcbcrConversion.h +++ b/src/gpu/vk/GrVkSamplerYcbcrConversion.h @@ -23,7 +23,7 @@ public: struct Key { Key() : fVkFormat(VK_FORMAT_UNDEFINED), fExternalFormat(0), fConversionKey(0) {} - Key(VkFormat vkFormat, uint64_t externalFormat, uint8_t conversionKey) { + Key(VkFormat vkFormat, int64_t externalFormat, uint8_t conversionKey) { memset(this, 0, sizeof(Key)); fVkFormat = vkFormat; fExternalFormat = externalFormat; @@ -31,7 +31,7 @@ public: } VkFormat fVkFormat; - uint64_t fExternalFormat; + int64_t fExternalFormat; uint8_t fConversionKey; bool operator==(const Key& that) const {