unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize

RESOLVED DUPLICATE of bug 603879

Status

()

--
trivial
RESOLVED DUPLICATE of bug 603879
8 years ago
a month ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
coverity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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;
(Assignee)

Comment 1

8 years ago
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   }
(Assignee)

Updated

8 years ago
Summary: unreachable code in harfbuzz HeadlessArrayOf::sanitize → unreachable code in harfbuzz HeadlessArrayOf::sanitize and GenericArrayOf::sanitize
(Assignee)

Comment 2

8 years ago
Created attachment 482489 [details] [diff] [review]
remove it
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #482489 - Flags: review?(jfkthame)
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...)
(Assignee)

Comment 4

8 years ago
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

8 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.
(Assignee)

Comment 6

8 years ago
i'd expect Coverity to say that 'array[0].santitize()' is unreachable [=deadcode] in your example....
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

8 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.
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 603879
You need to log in before you can comment on or make changes to this bug.