Closed
Bug 603346
Opened 14 years ago
Closed 14 years ago
unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
DUPLICATE
of bug 603879
People
(Reporter: timeless, Assigned: timeless)
References
(Blocks 1 open bug)
Details
(Keywords: coverity)
Attachments
(1 file)
1.43 KB,
patch
|
jfkthame
:
review-
|
Details | Diff | Splinter Review |
668 template <typename Type> 669 struct HeadlessArrayOf 670 { 684 inline bool sanitize (hb_sanitize_context_t *c) { 686 if (unlikely (!sanitize_shallow (c))) return false; 691 return true; this code is unreachable: 692 /* We do keep this code though to make sure the structs pointed 693 * to do have a simple sanitize(), ie. they do not reference 694 * other structs. */ 695 unsigned int count = len ? len - 1 : 0; 696 Type *a = array; 697 for (unsigned int i = 0; i < count; i++) 698 if (unlikely (!a[i].sanitize (c))) 699 return false; 700 return true;
550 template <typename LenType, typename Type> 551 struct GenericArrayOf 552 { 573 inline bool sanitize (hb_sanitize_context_t *c) { 574 TRACE_SANITIZE (); 575 if (unlikely (!sanitize_shallow (c))) return false; 580 return true; this code is unreachable: 581 /* We do keep this code though to make sure the structs pointed 582 * to do have a simple sanitize(), ie. they do not reference 583 * other structs. */ 584 unsigned int count = len; 585 for (unsigned int i = 0; i < count; i++) 586 if (array[i].sanitize (c)) 587 return false; 588 return true; 589 }
Summary: unreachable code in harfbuzz HeadlessArrayOf::sanitize → unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize
Comment 3•14 years ago
|
||
It looks from the comments here as though Behdad wants to keep that code around, at least for the time being, even though it's obviously unreachable. Is there any reason we should worry about it? (Actually, the dead code looks logically wrong in the GenericArrayOf case; I'd think it should be if (!array[i].sanitize(c)) return false; within the loop (the "!" is missing). So I suppose it's a good thing it doesn't get executed...)
People tend to assume code works. Conversely they sometimes will assume that an early return is accidental. As such keeping bad code in is a recipe for trouble. I happened to read: http://www.usenix.org/events/sec08/tech/slides/engler_slides.pdf last night. I think you'll find that it argues against dead code. This isn't the only piece of buggy dead code I hit in my most recent round either. Anyway, I don't own the code, you can do what you like.
Comment 5•14 years ago
|
||
Thanks Jonathan, fixed the missing negation. So, I do need to have the compiler check that array[0].sanitize() exists. I can of course do some other kind of unreachable code that the static-analyzers are less grumpy about, something like "return 0 ? array[0].sanitize() : true", but that doesn't change the bottom-line fact that I just want to make sure that function compiles without running it.
i'd expect Coverity to say that 'array[0].santitize()' is unreachable [=deadcode] in your example....
Comment 7•14 years ago
|
||
Comment on attachment 482489 [details] [diff] [review] remove it I think we should resolve this as WONTFIX, as I don't think there's any compelling reason for us to patch harfbuzz here. If Behdad chooses to modify how this is handled upstream, we'll pick up those changes, but it's likely that some "dead" code will be deliberately kept around in some form.
Attachment #482489 -
Flags: review?(jfkthame) → review-
Comment 8•14 years ago
|
||
Yeah. I'm playing with it now to see if I can get a way that makes gcc's -Wunreachable-code happy. No guarantees on coverity.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•