1241118 - [Static Analysis][Logically dead code] In function gfxScriptItemizer::Next from gfxScriptItemizer.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable that variable gc is only given a value once with HB_UNICODE_GENERAL_CATEGORY_UNASSIGNED so making the following code useless:

>>            if (gc == HB_UNICODE_GENERAL_CATEGORY_OPEN_PUNCTUATION) {
>>                uint32_t endPairChar = mozilla::unicode::GetMirroredChar(ch);
>>                if (endPairChar != ch) {
>>                    push(endPairChar, scriptCode);
>>                }
>>            } else if (gc == HB_UNICODE_GENERAL_CATEGORY_CLOSE_PUNCTUATION &&
>>                HasMirroredChar(ch))
>>            {
>>                while (STACK_IS_NOT_EMPTY() && TOP().endPairChar != ch) {
>>                    pop();
>>                }
>>
>>                if (STACK_IS_NOT_EMPTY()) {
>>                    sc = TOP().scriptCode;
>>                }
>>            }

and

>>            if (gc == HB_UNICODE_GENERAL_CATEGORY_CLOSE_PUNCTUATION &&
>>                HasMirroredChar(ch)) {
>>                pop();
>>            }

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame

Review commit: https://reviewboard.mozilla.org/r/31619/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31619/

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8709971 [details]
MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame

https://reviewboard.mozilla.org/r/31619/#review28315

See below; this code shouldn't be dead. If you want to morph this bug to fix the actual issue, fine; otherwise I'll post a patch shortly.

::: gfx/thebes/gfxScriptItemizer.cpp
(Diff revision 1)
> -            GetGeneralCategory(ch);

No, the real bug (which I introduced in bug 724538 -- oops!) is that we fail to set `gc` here, so the `GetGeneralCategory` call is pointless.

(Note the comment on the declaration/initialization of `gc`, above; that provides a strong clue about the intended behavior here, and it used to be true until I broke it recently!)

Comment 3

[Andi [:andi]]

Comment on attachment 8709971 [details]
MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31619/diff/1-2/

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8709971 [details]
MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame

https://reviewboard.mozilla.org/r/31619/#review28321

Thanks, that should fix it. (Please make sure you update the commit message accordingly, too.)

Comment 5

[Andi [:andi]]

Comment on attachment 8709971 [details]
MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31619/diff/2-3/

Comment 6

[Jonathan Kew [:jfkthame]]

https://reviewboard.mozilla.org/r/31619/#review28323

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c04144a61f3e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c04144a61f3e