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 <marc.mutz@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

4 files changed, 29 insertions, 2 deletions

diff --git a/src/corelib/global/qassert.cpp b/src/corelib/global/qassert.cpp
index 2e340ebbb2..4165429d4b 100644
--- a/src/corelib/global/qassert.cpp
+++ b/src/corelib/global/qassert.cpp
@@ -195,7 +195,25 @@ void qBadAlloc()
 
     In debug builds the condition is enforced by an assert to facilitate debugging.
 
-    \sa Q_ASSERT(), Q_ASSUME(), qFatal()
+    \note Use the macro Q_UNREACHABLE_RETURN() to insert return statements for
+    compilers that need them, without causing warnings for compilers that
+    complain about its presence.
+
+    \sa Q_ASSERT(), Q_ASSUME(), qFatal(), Q_UNREACHABLE_RETURN()
 */
 
+/*!
+    \macro void Q_UNREACHABLE_RETURN(...)
+    \relates <QtAssert>
+    \since 6.5
+
+    This is equivalent to
+    \code
+    Q_UNREACHABLE();
+    return __VA_ARGS__;
+    \endcode
+    except it omits the return on compilers that would warn about it.
+
+    \sa Q_UNREACHABLE()
+*/
 QT_END_NAMESPACE
diff --git a/src/corelib/global/qassert.h b/src/corelib/global/qassert.h
index c7ee13176a..28c6b6c8fc 100644
--- a/src/corelib/global/qassert.h
+++ b/src/corelib/global/qassert.h
@@ -70,6 +70,14 @@ inline T *q_check_ptr(T *p) { Q_CHECK_PTR(p); return p; }
         Q_UNREACHABLE_IMPL();\
     } while (false)
 
+#ifndef Q_UNREACHABLE_RETURN
+#  ifdef Q_COMPILER_COMPLAINS_ABOUT_RETURN_AFTER_UNREACHABLE
+#    define Q_UNREACHABLE_RETURN(...) Q_UNREACHABLE()
+#  else
+#    define Q_UNREACHABLE_RETURN(...) do { Q_UNREACHABLE(); return __VA_ARGS__; } while (0)
+#  endif
+#endif
+
 #define Q_ASSUME(Expr) \
     [] (bool valueOfExpression) {\
         Q_ASSERT_X(valueOfExpression, "Q_ASSUME()", "Assumption in Q_ASSUME(\"" #Expr "\") was not correct");\
diff --git a/src/corelib/global/qcompilerdetection.h b/src/corelib/global/qcompilerdetection.h
index 6dd76ce30a..35d0c5816b 100644
--- a/src/corelib/global/qcompilerdetection.h
+++ b/src/corelib/global/qcompilerdetection.h
@@ -49,6 +49,7 @@
 
 #if defined(__COVERITY__)
 #  define Q_CC_COVERITY
+#  define Q_COMPILER_COMPLAINS_ABOUT_RETURN_AFTER_UNREACHABLE
 #endif
 
 /* Symantec C++ is now Digital Mars */
diff --git a/src/corelib/global/qcompilerdetection.qdoc b/src/corelib/global/qcompilerdetection.qdoc
index 05a4b89f1d..377323a5eb 100644
--- a/src/corelib/global/qcompilerdetection.qdoc
+++ b/src/corelib/global/qcompilerdetection.qdoc
@@ -180,7 +180,7 @@
     This is useful since a missing break statement is often a bug, and some
     compilers can be configured to emit warnings when one is not found.
 
-    \sa Q_UNREACHABLE()
+    \sa Q_UNREACHABLE(), Q_UNREACHABLE_RETURN()
 */
 
 /*!