Closed Bug 1345478 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: andi, Assigned: fedepad, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, good-first-bug, Whiteboard: CID 1400801)

Attachments

(1 file)

Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;
59 bytes, text/x-review-board-request
bbouvier
: review+
Details 
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?
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.
(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.
(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 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 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 on attachment 8848899 [details]
Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

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

Done!
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+
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!
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
Thanks to you for helping in the process!
Sure, if I find some others bug I will take them ;)
https://hg.mozilla.org/mozilla-central/rev/1741441028b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.