Closed Bug 1241118 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1348504 )

User Story

The Static Analysis tool Coverity added 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();
>>            }


Until revision 279833 of gfxScriptItemizer.cpp, gc was update in the loop with:

>>            gc = charProps.mCategory;

Attachments

(1 file)

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();
>>            }
User Story: (updated)
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!)
Attachment #8709971 - Flags: review?(jfkthame)
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/
Attachment #8709971 - Flags: review?(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.)
Attachment #8709971 - Flags: review?(jfkthame) → review+
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/
Attachment #8709971 - Attachment description: MozReview Request: Bug 1241118 - removed dead code from gfxScriptItemizer::Next. r?jfkthame → MozReview Request: Bug 1241118 - add gc = GetGeneralCategory(ch) when sc == MOZ_SCRIPT_COMMON. r?jfkthame
Keywords: checkin-needed
Blocks: 724538
https://hg.mozilla.org/mozilla-central/rev/c04144a61f3e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.