From cc16b26ef4b9e7217ad819a31b9df55855a6f780 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 17:55:47 -0700 Subject: [PATCH 1/8] [constexpr] IntType See https://github.com/harfbuzz/harfbuzz/pull/2875 --- src/hb-algs.hh | 13 ++++++++----- src/hb-open-type.hh | 12 ++++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/hb-algs.hh b/src/hb-algs.hh index 05c98ef20..bc170b054 100644 --- a/src/hb-algs.hh +++ b/src/hb-algs.hh @@ -83,7 +83,8 @@ static inline constexpr uint16_t hb_uint16_swap (uint16_t v) static inline constexpr uint32_t hb_uint32_swap (uint32_t v) { return (hb_uint16_swap (v) << 16) | hb_uint16_swap (v >> 16); } -template struct BEInt; +template +struct BEInt; template struct BEInt { @@ -124,14 +125,16 @@ struct BEInt template struct BEInt { + static_assert (!hb_is_signed (Type), ""); public: BEInt () = default; constexpr BEInt (Type V) : v {uint8_t ((V >> 16) & 0xFF), - uint8_t ((V >> 8) & 0xFF), - uint8_t ((V ) & 0xFF)} {} + uint8_t ((V >> 8) & 0xFF), + uint8_t ((V ) & 0xFF)} {} + constexpr operator Type () const { return (v[0] << 16) - + (v[1] << 8) - + (v[2] ); } + + (v[1] << 8) + + (v[2] ); } private: uint8_t v[3]; }; template diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh index 28353e4e5..067107965 100644 --- a/src/hb-open-type.hh +++ b/src/hb-open-type.hh @@ -53,14 +53,18 @@ namespace OT { */ /* Integer types in big-endian order and no alignment requirement */ -template +template > struct IntType { typedef Type type; - typedef hb_conditional wide_type; - IntType& operator = (wide_type i) { v = i; return *this; } - operator wide_type () const { return v; } + IntType () = default; + explicit constexpr IntType (Wide V) : v {V} {} + IntType& operator = (Wide i) { v = i; return *this; } + operator Wide () const { return v; } + bool operator == (const IntType &o) const { return (Type) v == (Type) o.v; } bool operator != (const IntType &o) const { return !(*this == o); } From f4f35a4d5fd595bc6887b9951f1c92ce4d100fe8 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 22:28:32 -0700 Subject: [PATCH 2/8] [constexpr] Use initializer instead of assignment --- src/hb-ot-layout-common.hh | 6 ++---- src/hb-ot-layout-gsubgpos.hh | 7 +------ 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/hb-ot-layout-common.hh b/src/hb-ot-layout-common.hh index 4985d5e83..32a18efdd 100644 --- a/src/hb-ot-layout-common.hh +++ b/src/hb-ot-layout-common.hh @@ -1821,8 +1821,7 @@ struct ClassDefFormat1 } /* TODO Speed up, using set overlap first? */ /* TODO(iter) Rewrite as dagger. */ - HBUINT16 k; /* TODO(constexpr) use constructor to initialize. */ - k = klass; + HBUINT16 k {klass}; const HBUINT16 *arr = classValue.arrayZ; for (unsigned int i = 0; i < count; i++) if (arr[i] == k && glyphs->has (startGlyph + i)) @@ -1995,8 +1994,7 @@ struct ClassDefFormat2 } /* TODO Speed up, using set overlap first? */ /* TODO(iter) Rewrite as dagger. */ - HBUINT16 k; /* TODO(constexpr) use constructor to initialize. */ - k = klass; + HBUINT16 k {klass}; const RangeRecord *arr = rangeRecord.arrayZ; for (unsigned int i = 0; i < count; i++) if (arr[i].value == k && arr[i].intersects (glyphs)) diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh index aa46befbe..3a9656596 100644 --- a/src/hb-ot-layout-gsubgpos.hh +++ b/src/hb-ot-layout-gsubgpos.hh @@ -2323,12 +2323,7 @@ struct ChainRule { c->copy (len); for (const auto g : it) - { - /* TODO(constexpr) Simplify. */ - HBUINT16 gid; - gid = g; - c->copy (gid); - } + c->copy (HBUINT16 {g}); } ChainRule* copy (hb_serialize_context_t *c, From 567cedcc5f99aae2db9c7f124b7f3a6f4b5ec57d Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 22:09:15 -0700 Subject: [PATCH 3/8] Narrow down cast operators on IntType Say for USHORT, we were implementing casts from and to unsigned. With this change, we cast from and to uint16_t only. This allows compiler more opportunities to catch possible narrowing issues in the code. It needed a couple of fixes in the codebase though, because previously, if a USHORT was participating in arithmetic with signed numbers, eg. "u + 1", the result would have been unsigned. With this change, it would be signed. The correct fix is to update the code to read "u + 1u". That said, I think about conditionally adding back the cast out to signed/unsigned, to facilitate better type deduction. But I couldn't think of a real situation where that would help with anything. So I didn't add. Here's what it was: template , hb_enable_if (sizeof (Type) < sizeof (Type2))> operator hb_type_identity_t () const { return v; } https://github.com/harfbuzz/harfbuzz/pull/2875 --- src/hb-aat-layout-common.hh | 4 ++-- src/hb-aat-layout-morx-table.hh | 4 ++-- src/hb-open-type.hh | 9 ++++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/hb-aat-layout-common.hh b/src/hb-aat-layout-common.hh index 3e83673c8..d8223dc8e 100644 --- a/src/hb-aat-layout-common.hh +++ b/src/hb-aat-layout-common.hh @@ -576,7 +576,7 @@ struct StateTable if (unlikely (stop > states)) return_trace (false); for (const HBUSHORT *p = states; stop < p; p--) - num_entries = hb_max (num_entries, *(p - 1) + 1); + num_entries = hb_max (num_entries, *(p - 1) + 1u); state_neg = min_state; } } @@ -597,7 +597,7 @@ struct StateTable if (unlikely (stop < states)) return_trace (false); for (const HBUSHORT *p = &states[state_pos * num_classes]; p < stop; p++) - num_entries = hb_max (num_entries, *p + 1); + num_entries = hb_max (num_entries, *p + 1u); state_pos = max_state + 1; } } diff --git a/src/hb-aat-layout-morx-table.hh b/src/hb-aat-layout-morx-table.hh index 04027a61b..cf6766b04 100644 --- a/src/hb-aat-layout-morx-table.hh +++ b/src/hb-aat-layout-morx-table.hh @@ -337,9 +337,9 @@ struct ContextualSubtable const EntryData &data = entries[i].data; if (data.markIndex != 0xFFFF) - num_lookups = hb_max (num_lookups, 1 + data.markIndex); + num_lookups = hb_max (num_lookups, 1u + data.markIndex); if (data.currentIndex != 0xFFFF) - num_lookups = hb_max (num_lookups, 1 + data.currentIndex); + num_lookups = hb_max (num_lookups, 1u + data.currentIndex); } return_trace (substitutionTables.sanitize (c, this, num_lookups)); diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh index 067107965..1af4e6e64 100644 --- a/src/hb-open-type.hh +++ b/src/hb-open-type.hh @@ -54,16 +54,15 @@ namespace OT { /* Integer types in big-endian order and no alignment requirement */ template > + unsigned int Size = sizeof (Type)> struct IntType { typedef Type type; IntType () = default; - explicit constexpr IntType (Wide V) : v {V} {} - IntType& operator = (Wide i) { v = i; return *this; } - operator Wide () const { return v; } + explicit constexpr IntType (Type V) : v {V} {} + IntType& operator = (Type i) { v = i; return *this; } + operator Type () const { return v; } bool operator == (const IntType &o) const { return (Type) v == (Type) o.v; } bool operator != (const IntType &o) const { return !(*this == o); } From 09836013995cab2b9f07577a179ad7b024130467 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 22:33:17 -0700 Subject: [PATCH 4/8] Add back wider cast to IntType MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My local clang12 is fine, but many bots are not: ../src/hb-ot-cff1-table.hh: In instantiation of ‘bool CFF::Charset1_2::sanitize(hb_sanitize_context_t*, unsigned int) const [with TYPE = OT::IntType]’: ../src/hb-ot-cff1-table.hh:554:13: required from here ../src/hb-ot-cff1-table.hh:377:60: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (unlikely (!ranges[i].sanitize (c) || (num_glyphs < ranges[i].nLeft + 1))) ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ Enabling the extra cast operator mentioned in previous commit to see if that fixes this case. Again, I'd be happy to say "use 1u instead of 1" if this was universally erred on. But since some compilers happily compile this while others err, it would be a huge headache. Let's see... https://github.com/harfbuzz/harfbuzz/pull/2875 --- src/hb-open-type.hh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh index 1af4e6e64..ff12d4af5 100644 --- a/src/hb-open-type.hh +++ b/src/hb-open-type.hh @@ -64,6 +64,11 @@ struct IntType IntType& operator = (Type i) { v = i; return *this; } operator Type () const { return v; } + template , + hb_enable_if (sizeof (Type) < sizeof (Type2))> + operator hb_type_identity_t () const { return v; } + + bool operator == (const IntType &o) const { return (Type) v == (Type) o.v; } bool operator != (const IntType &o) const { return !(*this == o); } From d6bd00a488ace632d51748b028a0378a2bdaad2c Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 22:42:50 -0700 Subject: [PATCH 5/8] Revert back IntType out cast to signed/unsigned Previous commit didn't fix the bots. Putting it back now that I understand why I initially did the "Wide" casts. But only doing it for out-cast this time. This causes "narrowing" warnings whenever we are converting signed/unsigned to smaller HBUINT16 etc. But those are valuable warnings. We should address those separately instead of ignoring. Maybe we should start using uint16_t more liberally in the internal subsetter function signatures then. --- src/hb-open-type.hh | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh index ff12d4af5..3c6625376 100644 --- a/src/hb-open-type.hh +++ b/src/hb-open-type.hh @@ -62,12 +62,7 @@ struct IntType IntType () = default; explicit constexpr IntType (Type V) : v {V} {} IntType& operator = (Type i) { v = i; return *this; } - operator Type () const { return v; } - - template , - hb_enable_if (sizeof (Type) < sizeof (Type2))> - operator hb_type_identity_t () const { return v; } - + operator hb_conditional () const { return v; } bool operator == (const IntType &o) const { return (Type) v == (Type) o.v; } bool operator != (const IntType &o) const { return !(*this == o); } From 6c4bb60829d6e00647cb7ee74d816d648905cc3f Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 22 Feb 2021 22:45:32 -0700 Subject: [PATCH 6/8] Fix narrowing errors with recent changes --- src/hb-ot-layout-common.hh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hb-ot-layout-common.hh b/src/hb-ot-layout-common.hh index 32a18efdd..0a7e27b00 100644 --- a/src/hb-ot-layout-common.hh +++ b/src/hb-ot-layout-common.hh @@ -1788,7 +1788,7 @@ struct ClassDefFormat1 } template - bool collect_class (set_t *glyphs, unsigned int klass) const + bool collect_class (set_t *glyphs, unsigned klass) const { unsigned int count = classValue.len; for (unsigned int i = 0; i < count; i++) @@ -1806,7 +1806,7 @@ struct ClassDefFormat1 if (classValue[iter - start]) return true; return false; } - bool intersects_class (const hb_set_t *glyphs, unsigned int klass) const + bool intersects_class (const hb_set_t *glyphs, uint16_t klass) const { unsigned int count = classValue.len; if (klass == 0) @@ -1973,7 +1973,7 @@ struct ClassDefFormat2 return true; return false; } - bool intersects_class (const hb_set_t *glyphs, unsigned int klass) const + bool intersects_class (const hb_set_t *glyphs, uint16_t klass) const { unsigned int count = rangeRecord.len; if (klass == 0) From 83b66bfb665bb82fd03ae97c6f0e3eba01c0cba4 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Tue, 23 Feb 2021 13:04:25 -0700 Subject: [PATCH 7/8] Another try to fix narrowing error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ../src/hb-ot-layout-gsubgpos.hh: In instantiation of ‘void OT::ChainRule::serialize_array(hb_serialize_context_t*, OT::HBUINT16, Iterator) const [with Iterator = hb_map_iter_t >, const hb_map_t*&, (hb_function_sortedness_t)0, 0>; typename hb_enable_if::value>::type* = 0; OT::HBUINT16 = OT::IntType]’: ../src/hb-ot-layout-gsubgpos.hh:2341:30: required from here ../src/hb-ot-layout-gsubgpos.hh:2326:15: error: narrowing conversion of ‘(unsigned int)g’ from ‘unsigned int’ to ‘short unsigned int’ inside { } [-Werror=narrowing] c->copy (HBUINT16 {g}); ~~~~~~~~^~~~~~~~~~~~~~ https://github.com/harfbuzz/harfbuzz/pull/2875 --- src/hb-ot-layout-gsubgpos.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh index 3a9656596..0f55a10a6 100644 --- a/src/hb-ot-layout-gsubgpos.hh +++ b/src/hb-ot-layout-gsubgpos.hh @@ -2323,7 +2323,7 @@ struct ChainRule { c->copy (len); for (const auto g : it) - c->copy (HBUINT16 {g}); + c->copy ((HBUINT16) g); } ChainRule* copy (hb_serialize_context_t *c, From 486da35cc0954505db57b2e0f5b5b0b45a7c4007 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Tue, 23 Feb 2021 13:58:14 -0700 Subject: [PATCH 8/8] m Add comments to IntType cast out operator Okay, bots seem to be happy. Merging. --- src/hb-open-type.hh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/hb-open-type.hh b/src/hb-open-type.hh index 3c6625376..dc0ae1d98 100644 --- a/src/hb-open-type.hh +++ b/src/hb-open-type.hh @@ -62,6 +62,8 @@ struct IntType IntType () = default; explicit constexpr IntType (Type V) : v {V} {} IntType& operator = (Type i) { v = i; return *this; } + /* For reason we define cast out operator for signed/unsigned, instead of Type, see: + * https://github.com/harfbuzz/harfbuzz/pull/2875/commits/09836013995cab2b9f07577a179ad7b024130467 */ operator hb_conditional () const { return v; } bool operator == (const IntType &o) const { return (Type) v == (Type) o.v; }