[Static Analysis][Logically Dead Code] In function PrintSingleError

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: fedepad, Mentored)

Tracking

(Blocks 1 bug, {coverity, good-first-bug})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: CID 1400801)

Attachments

(1 attachment)

In the following context:

>>    if (kind != PrintErrorKind::Error) {
>>        const char* kindPrefix = nullptr;
>>        switch (kind) {
>>          case PrintErrorKind::Error:
>>            break;
>>          case PrintErrorKind::Warning:
>>            kindPrefix = "warning";
>>            break;
>>          case PrintErrorKind::StrictWarning:
>>            kindPrefix = "strict warning";
>>            break;
>>          case PrintErrorKind::Note:
>>            kindPrefix = "note";
>>            break;
>>        }
>>
>>        UniquePtr<char> tmp(JS_smprintf("%s%s: ", prefix ? prefix.get() : "", kindPrefix));
>>        prefix = Move(tmp);
>>    }

'case PrintErrorKind::Error' is useless and should be removed.
I think it makes sense to have the explicit case instead of a default, so that next time somebody adds an enum value, they'll get a warning that it's not handled in this switch case. Maybe worth to MOZ_CRASH if it's hit?
Reporter

Comment 2

2 years ago
I agree with you but in this context the explicit case for PrintErrorKind::Error will never be hit or were you referring to something else?
To continue your idea we could also have something similar to:

>>default:
>>  MOZ_CRASH(...);

In this way we can any other enum members that would be added and not handled here.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> I agree with you but in this context the explicit case for
> PrintErrorKind::Error will never be hit or were you referring to something
> else?

Yes, that's what I'm saying: the explicit case should have a MOZ_CRASH("unreachable").

> To continue your idea we could also have something similar to:
> 
> >>default:
> >>  MOZ_CRASH(...);
> 
> In this way we can any other enum members that would be added and not
> handled here.

No, that would happen at runtime, which is slightly worse than seeing the warning at compile time.
Reporter

Comment 4

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> > I agree with you but in this context the explicit case for
> > PrintErrorKind::Error will never be hit or were you referring to something
> > else?
> 
> Yes, that's what I'm saying: the explicit case should have a
> MOZ_CRASH("unreachable").
> 
> > To continue your idea we could also have something similar to:
> > 
> > >>default:
> > >>  MOZ_CRASH(...);
> > 
> > In this way we can any other enum members that would be added and not
> > handled here.
> 
> No, that would happen at runtime, which is slightly worse than seeing the
> warning at compile time.
Yes that's a bit worse since it happens during runtime, thats why i wanted to confirm it with you.
Assignee

Comment 5

2 years ago
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #4)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> > (In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)
> > > I agree with you but in this context the explicit case for
> > > PrintErrorKind::Error will never be hit or were you referring to something
> > > else?
> > 
> > Yes, that's what I'm saying: the explicit case should have a
> > MOZ_CRASH("unreachable").
> > 
> > > To continue your idea we could also have something similar to:
> > > 
> > > >>default:
> > > >>  MOZ_CRASH(...);
> > > 
> > > In this way we can any other enum members that would be added and not
> > > handled here.
> > 
> > No, that would happen at runtime, which is slightly worse than seeing the
> > warning at compile time.
> Yes that's a bit worse since it happens during runtime, thats why i wanted
> to confirm it with you.

I will prepare a patch for this. Should I put you as a reviewer for this?

Thanks.
Flags: needinfo?(bpostelnicu)
Comment hidden (mozreview-request)
Reporter

Comment 7

2 years ago
Comment on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

Forwarding this to Benjamin Bouvier.
Flags: needinfo?(bpostelnicu)
Attachment #8848899 - Flags: review?(bpostelnicu) → review?(bbouvier)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

https://reviewboard.mozilla.org/r/121766/#review123906

Looks good to me, but can you change the changeset summary (first line) to something more natural, like:

Bug XXX - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError; r?bbouvier

Once this is done, please re-flag me for review. Thanks for fixing this bug!
Attachment #8848899 - Flags: review?(bbouvier)
Comment hidden (mozreview-request)
Assignee

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

https://reviewboard.mozilla.org/r/121766/#review123906

Done!

Comment 11

2 years ago
mozreview-review
Comment on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

https://reviewboard.mozilla.org/r/121766/#review123948

Looks good to me, thank you!
Attachment #8848899 - Flags: review?(bbouvier) → review+
Assignee

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

https://reviewboard.mozilla.org/r/121766/#review123948

You're welcome! Thank you!

Comment 13

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1741441028b5
Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError; r=bbouvier
After a try build showed the build was still successful with your patch, I've requested landing it on your behalf. Thanks for fixing it, and don't hesitate taking other interesting bugs :)
Assignee: nobody → federico_padua
Status: NEW → ASSIGNED
Assignee

Comment 15

2 years ago
Thanks to you for helping in the process!
Sure, if I find some others bug I will take them ;)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1741441028b5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.