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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 2

[Andi [:andi]]

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.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(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.

Comment 4

[Andi [:andi]]

(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.

Comment 5

[Federico Padua [:fedepad]]

(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.

Comment 6

[Federico Padua [:fedepad]]

Attachment: Bug 1345478 - Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError;

This patch explicitly calls MOZ_CRASH() in the switch case
PrintErrorKind::Error that is never reached inside the function
PrintSingleError in the file js/src/jscntxt.cpp.
Explicitly handling all the possible cases avoids the compiler to issue
warnings for them: a non handled case in the switch code that might derive
from additions to the enum class PrintErrorKind can be caught at compile time
rather then having a default switch case for handling it, which instead would
be called at runtime.

Review commit: https://reviewboard.mozilla.org/r/121766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121766/

Comment 7

[Andi [:andi]]

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

Forwarding this to Benjamin Bouvier.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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!

Comment 9

[Federico Padua [:fedepad]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121766/diff/1-2/

Comment 10

[Federico Padua [:fedepad]]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

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!

Comment 12

[Federico Padua [:fedepad]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1741441028b5
Make the PrintErrorKind::Error case explicitly unreachable in PrintSingleError; r=bbouvier

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

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 :)

Comment 15

[Federico Padua [:fedepad]]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1741441028b5