[Static Analysis][Coverity] Kill structurally dead code in NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN

RESOLVED FIXED in Firefox 57

Status

Firefox Build System
Source Code Analysis
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla57
coverity

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Suppress structurally dead code generated by Coverity in NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN, by modifying the modelling file.
Comment hidden (mozreview-request)
did you try if it is working?
(Assignee)

Comment 3

9 months ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> did you try if it is working?

It eliminated one issue 2 month ago. But i haven't updated the version from m-c.

Comment 4

9 months ago
mozreview-review
Comment on attachment 8906929 [details]
Bug 1398785 - Kill structurally dead code in NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN generated by Coverity.

https://reviewboard.mozilla.org/r/178668/#review183666

::: tools/coverity/model.cpp:63
(Diff revision 1)
> +#define NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(_class)                   \
> +  NS_IMETHODIMP_(bool)                                                         \
> +  NS_CYCLE_COLLECTION_CLASSNAME(_class)::CanSkipThisReal(void* p)              \
> +  {                                                                            \
> +    __coverity_panic__();                                                      \
> +    _class* tmp = DowncastCCParticipant<_class>(p);

What will happen if this changes in the base code?
I am concerned that we might miss changes here, don't you think?
(Assignee)

Comment 5

9 months ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> Comment on attachment 8906929 [details]
> Bug 1398785 - Kill structurally dead code in
> NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN generated by Coverity.
> 
> https://reviewboard.mozilla.org/r/178668/#review183666
> 
> ::: tools/coverity/model.cpp:63
> (Diff revision 1)
> > +#define NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(_class)                   \
> > +  NS_IMETHODIMP_(bool)                                                         \
> > +  NS_CYCLE_COLLECTION_CLASSNAME(_class)::CanSkipThisReal(void* p)              \
> > +  {                                                                            \
> > +    __coverity_panic__();                                                      \
> > +    _class* tmp = DowncastCCParticipant<_class>(p);
> 
> What will happen if this changes in the base code?
> I am concerned that we might miss changes here, don't you think?
I don't think that the current state of the code will change in the near to medium feature since the definition above is part of a meta-data that defines a member function header, and if it changes it would mean that a major refactor will be done to the code.
Let's say that this will change, in this case the only thing that Coverity will ignore is the branch of the definition skipping thus the analysis of the rest of the body.

Comment 6

9 months ago
mozreview-review
Comment on attachment 8906929 [details]
Bug 1398785 - Kill structurally dead code in NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN generated by Coverity.

https://reviewboard.mozilla.org/r/178668/#review183674

OK, let's try then!
Attachment #8906929 - Flags: review?(sledru) → review+

Comment 7

9 months ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4cedbf13eb8
Kill structurally dead code in NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN generated by Coverity. r=sylvestre
https://hg.mozilla.org/mozilla-central/rev/c4cedbf13eb8
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.