1608396 - [Automated review] Coverity warnings when using if constexpr

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P1)

Description

[Simon Giesecke [:sg] [he/him]]

+++ This bug was initially created as a clone of Bug #1605181 +++

Bug 1605181 fixed clang-tidy, but the Coverity issue is still there, as can be seen on https://phabricator.services.mozilla.com/D57993#1813370, for example.

Phabricator URL: https://phabricator.services.mozilla.com/D57799#1763765

In a construct using if constexpr, I get analyzer warnings from both clang-tidy and coverity, which do not seem to make sense:

Checker reliability is medium, meaning that the false positive ratio is medium.
An "if" statement with no "then" or "else" is suspicious.
The path that leads to this defect is:

    xpcom/tests/gtest/TestThreadUtils.cpp:267:
        stray_semicolon: An "if" statement with no "then" or "else" is suspicious..

    xpcom/tests/gtest/TestThreadUtils.cpp:267:
        remediation: Is the ";" after "if (false)" extraneous, or is the "if" itself unnecessary?.


Warning: Potentially unintended semicolon [clang-tidy: bugprone-suspicious-semicolon]
Checker reliability is high, meaning that the false positive ratio is low.


Warning: Statement should be inside braces [clang-tidy: readability-braces-around-statements]
Checker reliability is high, meaning that the false positive ratio is low.

The offending code is

if constexpr (RunnableFactory::SupportsCopyWithDeletedMove) {
  // ...
}

where RunnableFactory::SupportsCopyWithDeletedMove is a bool depending on a template argument.

Comment 1

[Andi [:andi]]

Thanks for reporting this, please see bug 1608399. This is due that Coverity lacks behind a little bit with the trends from C++ and it's support for C++17 has some issues. I hope that by upgrading to the latest version 2019.12 we will have this sorted out, also this must be complemented by using clang-9 on the analysis side.

Comment 2

[Andi [:andi]]

This has been fixed in the latest coverity analysis release, 2020.03.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2)

This has been fixed in the latest coverity analysis release, 2020.03.

Have we updated to this release? Because I am still seeing these warnings.

Comment 4

[Andi [:andi]]

Yes, but unfortunately it doesn't fix all of the issues. I've contacted Synopsis regarding this.

Comment 5

[Andi [:andi]]

Attachment: Bug 1608396 - for `Coverity Analysis` disable checker `STRAY_SEMICOLON` due to the noise that it creates with `if constexpr`. r=sylvestre

We disable this checker until Synopsis fixes this or provides a valid workaround.

Comment 6

[Andi [:andi]]

We disable this checker until Synopsis provides a valid workaround, please note it will take a while until this propagates.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25060711bef4
for `Coverity Analysis` disable checker `STRAY_SEMICOLON` due to the noise that it creates with `if constexpr`. r=froydnj

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/25060711bef4