Suppress clang and MSVC warnings in gfx/harfbuzz

RESOLVED WORKSFORME

Status

()

Core
Graphics: Text
RESOLVED WORKSFORME
3 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8675358 [details] [diff] [review]
MOZ_FALLTHROUGH_harfbuzz.patch

Suppress clang's (optional) -Wimplicit-fallthrough warnings about switch cases that fall through without a break or return statement. The warnings can also be suppressed with a clang-specific C++11 annotation [[clang::fallthrough]] (aka MOZ_FALLTHROUGH macro from bug 1215411).

switch (foo) {
  case 1: // These cases have no code. No fallthrough annotations are needed.
  case 2:
  case 3:
    foo = 4; // This case has code, so a fallthrough annotation is needed:
    [[clang::fallthrough]]; // or MOZ_FALLTHROUGH;
  default:
    return foo;
}

gfx/harfbuzz/src/hb-ot-shape-fallback.cc:229:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/harfbuzz/src/hb-ot-shape-fallback.cc:263:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels
gfx/harfbuzz/src/hb-ot-shape-fallback.cc:283:5 [-Wimplicit-fallthrough] unannotated fall-through between switch labels

And suppress MSVC x64 warnings about 32-bit shifts. I reported these MSVC warnings to the upstream harfbuzz repo:

https://github.com/behdad/harfbuzz/issues/146

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(240) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(250) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(966) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1518) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1672) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc(1819) : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?).
Attachment #8675358 - Flags: review?(jdaggett)
(Assignee)

Comment 1

3 years ago
Created attachment 8675444 [details] [diff] [review]
1215894_suppress-harfbuzz-warnings-v2.patch

Also suppress clang and gcc's -Wshadow warnings and gcc's -Wshadow-local and -Wshadow-compatible-local warnings. These -Wshadow* warnings are not enabled by default, but I am slowly fixing them in other bugs. Do you prefer that I fix these warnings upstream instead of suppressing them in mozilla-central?

gfx/harfbuzz/src/hb-open-type-private.hh:382:40 [-Wshadow] declaration shadows a field of 'OT::hb_serialize_context_t'
gfx/harfbuzz/src/hb-open-type-private.hh:478:31 [-Wshadow] declaration shadows a field of 'OT::hb_serialize_context_t'
gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh:361:48 [-Wshadow] declaration shadows a field of 'OT::hb_apply_context_t'
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1250:23 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1625:26 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:459:23 [-Wshadow] declaration shadows a local variable
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:524:23 [-Wshadow] declaration shadows a local variable

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1222:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1254:23 [-Wshadow-local] declaration of 'info' shadows a previous local
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:426:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-myanmar.cc:458:23 [-Wshadow-local] declaration of 'info' shadows a previous local
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:483:20 [-Wshadow-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-use.cc:516:23 [-Wshadow-local] declaration of 'info' shadows a previous local

gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1595:23 [-Wshadow-compatible-local] shadowed declaration is here
gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:1634:26 [-Wshadow-compatible-local] declaration of 'i' shadows a previous local
Attachment #8675358 - Attachment is obsolete: true
Attachment #8675358 - Flags: review?(jdaggett)
Attachment #8675444 - Flags: review?(jdaggett)

Comment 2

3 years ago
Comment on attachment 8675444 [details] [diff] [review]
1215894_suppress-harfbuzz-warnings-v2.patch

Punting to Jonathan. My feeling is we should get these upstream but I'll defer to his opinion.
Attachment #8675444 - Flags: review?(jdaggett) → review?(jfkthame)
Behdad, are these warnings that you're willing to fix upstream, or do we need to just suppress them locally?
Flags: needinfo?(mozilla)

Comment 4

3 years ago
Yes, I'm upstreaming these now.  Can even get a release out today.
Flags: needinfo?(mozilla)

Comment 5

3 years ago
These should all be fixed.  I'll roll a release soon, but would be nice if someone can test them.
(Assignee)

Comment 6

3 years ago
Comment on attachment 8675444 [details] [diff] [review]
1215894_suppress-harfbuzz-warnings-v2.patch

Thanks, Behdad! I can test your changes soon. There is no rush to make a new release for these warnings. :)
Attachment #8675444 - Flags: review?(jfkthame)
(Assignee)

Updated

2 years ago
Blocks: 1253170

Comment 7

2 years ago
This should have been fixed by harfbuzz rolls since October.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.