From fc7676769251a27cbbc6d40d68f04bfe38511a5b Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 22 Sep 2022 11:39:44 +0200 Subject: Long live Q_UNREACHABLE_RETURN()! This is a combination of Q_UNREACHABLE() with a return statement. ATM, the return statement is unconditionally included. If we notice that some compilers warn about return after __builtin_unreachable(), then we can map Q_UNREACHABLE_RETURN(...) to Q_UNREACHABLE() without having to touch all the code that uses explicit Q_UNREACHABLE() + return. The fact that Boost has BOOST_UNREACHABLE_RETURN() indicates that there are compilers that complain about a lack of return after Q_UNREACHABLE (we know that MSVC, ICC, and GHS are among them), as well as compilers that complained about a return being present (Coverity). Take this opportunity to properly adapt to Coverity, by leaving out the return statement on this compiler. Apply the macro around the code base, using a clang-tidy transformer rule: const std::string unr = "unr", val = "val", ret = "ret"; auto makeUnreachableReturn = cat("Q_UNREACHABLE_RETURN(", ifBound(val, cat(node(val)), cat("")), ")"); auto ignoringSwitchCases = [](auto stmt) { return anyOf(stmt, switchCase(subStmt(stmt))); }; makeRule( stmt(ignoringSwitchCases(stmt(isExpandedFromMacro("Q_UNREACHABLE")).bind(unr)), nextStmt(returnStmt(optionally(hasReturnValue(expr().bind(val)))).bind(ret))), {changeTo(node(unr), cat(makeUnreachableReturn, ";")), // TODO: why is the ; lost w/o this? changeTo(node(ret), cat(""))}, cat("use ", makeUnreachableReturn)) ); where nextStmt() is copied from some upstream clang-tidy check's private implementation and subStmt() is a private matcher that gives access to SwitchCase's SubStmt. A.k.a. qt-use-unreachable-return. There were some false positives, suppressed them with NOLINTNEXTLINE. They're not really false positiives, it's just that Clang sees the world in one way and if conditonal compilation (#if) differs for other compilers, Clang doesn't know better. This is an artifact of matching two consecutive statements. I haven't figured out how to remove the empty line left by the deletion of the return statement, if it, indeed, was on a separate line, so post-processed the patch to remove all the lines matching ^\+ *$ from the diff: git commit -am meep git reset --hard HEAD^ git diff HEAD..HEAD@{1} | sed '/^\+ *$/d' | recountdiff - | patch -p1 [ChangeLog][QtCore][QtAssert] Added Q_UNREACHABLE_RETURN() macro. Change-Id: I9782939f16091c964f25b7826e1c0dbd13a71305 Reviewed-by: Marc Mutz Reviewed-by: Thiago Macieira --- src/corelib/kernel/qmetatype.cpp | 3 +-- src/corelib/kernel/qpropertyprivate.h | 3 +-- src/corelib/kernel/qvariant.cpp | 6 ++---- 3 files changed, 4 insertions(+), 8 deletions(-) (limited to 'src/corelib/kernel') diff --git a/src/corelib/kernel/qmetatype.cpp b/src/corelib/kernel/qmetatype.cpp index 5ad7967605..1047c8c0aa 100644 --- a/src/corelib/kernel/qmetatype.cpp +++ b/src/corelib/kernel/qmetatype.cpp @@ -2006,8 +2006,7 @@ static bool convertToEnum(QMetaType fromType, const void *from, QMetaType toType *static_cast(to) = value; return true; default: - Q_UNREACHABLE(); - return false; + Q_UNREACHABLE_RETURN(false); } } diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index 966821fa67..4387f0fbeb 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -194,8 +194,7 @@ struct BindingFunctionVTable return true; } else { // Our code will never instantiate this - Q_UNREACHABLE(); - return false; + Q_UNREACHABLE_RETURN(false); } }, /*destroy*/[](void *f){ static_cast(f)->~Callable(); }, diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 5e949487db..3fb80f203e 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -2261,8 +2261,7 @@ static bool integralEquals(uint promotedType, const QVariant::Private *d1, const if (promotedType == QMetaType::ULongLong) return qulonglong(l1) == qulonglong(l2); - Q_UNREACHABLE(); - return 0; + Q_UNREACHABLE_RETURN(0); } namespace { @@ -2306,8 +2305,7 @@ static std::optional integralCompare(uint promotedType, const QVariant::Pri if (promotedType == QMetaType::ULongLong) return spaceShip(l1, l2); - Q_UNREACHABLE(); - return 0; + Q_UNREACHABLE_RETURN(0); } static std::optional numericCompare(const QVariant::Private *d1, const QVariant::Private *d2) -- cgit v1.2.3