Last Comment Bug 780680 - gfxHarfBuzzShaper.cpp:1197:21: warning: variable ‘prevGlyphCharIndex’ set but not used [-Wunused-but-set-variable]
: gfxHarfBuzzShaper.cpp:1197:21: warning: variable ‘prevGlyphCharIndex’ set but...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla17
Assigned To: Daniel Holbert [:dholbert]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: buildwarning 249159
  Show dependency treegraph
 
Reported: 2012-08-06 12:14 PDT by Daniel Holbert [:dholbert]
Modified: 2012-08-07 07:38 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.46 KB, patch)
2012-08-06 12:18 PDT, Daniel Holbert [:dholbert]
jfkthame: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-08-06 12:14:55 PDT
../../../mozilla/gfx/thebes/gfxHarfBuzzShaper.cpp: In member function ‘nsresult gfxHarfBuzzShaper::SetGlyphsFromRun(gfxContext*, gfxShapedWord*, hb_buffer_t*)’:
../../../mozilla/gfx/thebes/gfxHarfBuzzShaper.cpp:1197:21: warning: variable ‘prevGlyphCharIndex’ set but not used [-Wunused-but-set-variable]

This is from bug 249159 part 4:
  https://hg.mozilla.org/mozilla-central/rev/1dbfff7cf0ab
which removed the last usage of this variable.
Comment 1 Daniel Holbert [:dholbert] 2012-08-06 12:18:39 PDT
Created attachment 649343 [details] [diff] [review]
fix
Comment 2 Daniel Holbert [:dholbert] 2012-08-06 12:20:59 PDT
(In reply to Daniel Holbert [:dholbert] from comment #0)
> This is from bug 249159 part 4:
>   https://hg.mozilla.org/mozilla-central/rev/1dbfff7cf0ab
> which removed the last usage of this variable.

(Incidentally, gfxCoreTextShaper.cpp has some veeeerry similar-
https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxCoreTextShaper.cpp#376
Comment 3 Daniel Holbert [:dholbert] 2012-08-06 12:29:27 PDT
(In reply to Daniel Holbert [:dholbert] from comment #2)
> (Incidentally, gfxCoreTextShaper.cpp has some veeeerry similar-
> https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxCoreTextShaper.
> cpp#376

(erm, probably ignore that -- didn't mean to post it.  I was starting to compose a comment about very-similar code (prevGlyphCharIndex, which is used to set "inOrder", which is used to in a call to g.SetComplex()), and I was wondering if that code might've wanted the same treatment that 1dbfff7cf0ab provided to gfxHarfBuzzShaper.cpp.  But I'm guessing it wasn't missed & this is a just a HarfBuzz vs. CoreText difference...?)
Comment 4 Jonathan Kew (:jfkthame) 2012-08-06 12:49:41 PDT
Comment on attachment 649343 [details] [diff] [review]
fix

Yes, that variable is clearly obsolete here; let's kill it.

As for the similar-looking code in gfxCoreTextShaper - it's possible that we could/should make a comparable change there, but I wouldn't want to attempt that without careful re-reading of the code and creating testcases specifically designed to hit the various edge cases. (This stuff is a bit hairy, and the underlying shapers don't necessarily behave in exactly the same way...) Let's leave well alone unless someone runs across a specific issue that prompts us to revisit it.
Comment 5 Daniel Holbert [:dholbert] 2012-08-06 13:11:28 PDT
Sounds good, thanks!

I'll land this once m-i reopens.
Comment 6 Daniel Holbert [:dholbert] 2012-08-06 13:39:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/759748513d64
Comment 7 Ed Morley [:emorley] 2012-08-07 07:38:04 PDT
https://hg.mozilla.org/mozilla-central/rev/759748513d64

Note You need to log in before you can comment on or make changes to this bug.