bugprone-branch-clone false positive with template functions and constexpr

[pavelkryukov]

Bugzilla Link	44229
Resolution	FIXED
Resolved on	Jan 22, 2020 16:07
Version	unspecified
OS	All
Blocks	#43900
CC	@JonasToth,@zmodem,@pavelkryukov
Fixed by commit(s)	rGf9c46229e4ac29053747c96e08c574c6c48d544b

Extended Description

Consider following example:

cat tidy-error.cpp

template<int y, typename T> void foo(T&&...);

template
void bar()
{
if constexpr ( x == 2)
foo<6>( 3);
else if constexpr ( x == 3)
foo<8>( 4.5);
else
foo<23>( "hello");
}

template void bar<2>();
template void bar<0>();

The expected output is no error, however:

clang-tidy-9 tidy-error.cpp -checks='bugprone-branch-clone' -extra-arg=-std=c++17

1 warning generated.
tidy-error.cpp:7:9: warning: repeated branch in conditional chain [bugprone-branch-clone]
foo<6>( 3);
^
/mnt/d/Dev/tidy-error.cpp:7:12: note: end of the original
foo<6>( 3);
^
/mnt/d/Dev/tidy-error.cpp:9:9: note: clone 1 starts here
foo<8>( 4.5);
^

[llvmbot]

I just ran into the same issue with the following code:

if constexpr (std::is_signed_v<T1> && std::is_unsigned_v<T2>) {
    if (lhs.get() < 0) {
        return false;
    }

    return static_cast<std::uintmax_t>(lhs.get()) == rhs.get();
}
else if constexpr (std::is_unsigned_v<T1> && std::is_signed_v<T2>) {
    if (rhs.get() < 0) {
        return false;
    }

    return lhs.get() == static_cast<std::uintmax_t>(rhs.get());
}
else {
    return lhs.get() == rhs.get();
}

[llvmbot]

If I re-write the function, I can make this error disappear, so I think it has something to do with the use of && or else if.

if constexpr (std::is_signed_v<T1>) {
    if constexpr (std::is_unsigned_v<T2>) {
        if (lhs.get() < 0) {
            return false;
        }

        return static_cast<std::uintmax_t>(lhs.get()) == rhs.get();
    }
    else {
        return lhs.get() == rhs.get();
    }
}
else  {
    if constexpr (std::is_unsigned_v<T2>) {
        return lhs.get() == rhs.get();
    }
    else {
        if (rhs.get() < 0) {
            return false;
        }

        return lhs.get() == static_cast<std::uintmax_t>(rhs.get());
    }
}

[llvmbot]

This is caused by the template instantiator folding away false branches of if constexpr construct to a null statement. The branches get folded away in the order they appear until a true branch is found. Therefore if you have a 3 or more branch if constexpr in an instantiated template and the condition fails for the first 2 branches, they will both be folded away and then caught as a branch clone.
The simple fix is to disregard all if constexpr statements, which is a simple task of adding unless(isConstexpr()) to the if matcher.
A more correct fix would be to disregard any branches that are in an if constexpr where the condition evaluates to false.

[llvmbot]

Fixed in https://reviews.llvm.org/rGf9c46229e4ac29053747c96e08c574c6c48d544b

[llvmbot]

Reopened, so that release managers see this is not in the release branch yet and that it can be backported.

Once that is done, the bug should be closed again.

[zmodem]

Fixed in https://reviews.llvm.org/rGf9c46229e4ac29053747c96e08c574c6c48d544b

That was cherry-picked in 1f98c2b

[zmodem]

mentioned in issue #43900