Closed Bug 1551098 Opened 8 months ago Closed 8 months ago

[Automated review] Coverity noise is too much

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emilio, Assigned: andi)

References

(Blocks 1 open bug)

Details

Phabricator URL: https://phabricator.services.mozilla.com/D30830 (but many others too, really)

  • Coverity warns for a lot of code that I haven't touched in my patch.

  • Coverity doesn't seem to understand MOZ_ASSERT / NS_ASSERTIONs that assert that a pointer is non-null, causing a lot of noise. For example, two of the messages in that CL is about dereferencing null pointers that are asserted against on the line below. Those pointers being null would be a logic error.

Also:

  • Coverity bugs me each time I update the CL, even if the same warning was emitted before.
Assignee: nobody → bpostelnicu
Duplicate of this bug: 1551037

FWIW, Coverity also doesn't understand basic template, but still attempts to make apparently wrong suggestion:

See https://phabricator.services.mozilla.com/D29992.

For a simple template with only a few lines, where a parameter is used to store output (*aRes = ...):

template <class... Tags>
bool ResolveAll(const SVGElement* aElement,
                details::AlwaysFloat<Tags>*... aRes) {
  if (nsIFrame const* f = aElement->GetPrimaryFrame()) {
    using dummy = int[];
    (void)dummy{0, (*aRes = ResolveWith<Tags>(*f->Style(), aElement), 0)...};
    return true;
  }
  return false;
}

Coverity tells me to mark it as const:

Warning: Pointer parameter 'aRes' can be pointer to const [clang-tidy: readability-non-const-parameter]
Checker reliability (false positive risk) is high.

This warning appears over and over again. It's really annoying. I think we should at least have the option to manually mark a Coverity suggestion as wrong, so that it won't appear again.

@violet.bugrepor the issue that you report is not related with Coverity analyser, but it's a clang-tidy checker. Besides the checker result you have it's reliability, do you think we should alter the reliability of this checker?

Besides the checker result you have it's reliability, do you think we should alter the reliability of this checker?

I'm not sure whether the checker happens to have trouble with this particular piece of code, or it is incapable of analyzing template in general. So I don't have an opinion on that.

I think the important thing is that user should be able to silence a particular warning, so that it doesn't appear over and over again, after every change. If that could be done, then some false positive doesn't matter.

No longer regressed by: coverity-analysis

Closing this as it has been addressed in other bugs and It was fixed since last week, and the fix is present in the latest release of releneg-services.

Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.