Skip to content

Commit

Permalink
Long live Q_UNREACHABLE_RETURN()!
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-by: Thiago Macieira <[email protected]>
  • Loading branch information
marcmutz committed Oct 15, 2022
1 parent 16dbbc8 commit fc76767
Showing 55 changed files with 154 additions and 221 deletions.
13 changes: 13 additions & 0 deletions src/corelib/doc/snippets/code/src_corelib_global_qglobal.cpp
Original file line number Diff line number Diff line change
@@ -667,6 +667,19 @@ bool readConfiguration(const QFile &file)
}
//! [qunreachable-switch]

//! [qunreachable-return]
switch (shape) {
case Rectangle:
return rectangle();
case Triangle:
return triangle();
case Circle:
return circle();
case NumShapes:
Q_UNREACHABLE_RETURN(nullptr);
}
//! [qunreachable-return]

//! [qt-version-check]
#include <QtGlobal>

20 changes: 19 additions & 1 deletion src/corelib/global/qassert.cpp
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions src/corelib/global/qassert.h
Original file line number Diff line number Diff line change
@@ -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");\
1 change: 1 addition & 0 deletions src/corelib/global/qcompilerdetection.h
Original file line number Diff line number Diff line change
@@ -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 */
2 changes: 1 addition & 1 deletion src/corelib/global/qcompilerdetection.qdoc
Original file line number Diff line number Diff line change
@@ -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()
*/

/*!
3 changes: 1 addition & 2 deletions src/corelib/io/qprocess.cpp
Original file line number Diff line number Diff line change
@@ -1903,8 +1903,7 @@ void QProcess::setProcessState(ProcessState state)
*/
auto QProcess::setupChildProcess() -> Use_setChildProcessModifier_Instead
{
Q_UNREACHABLE();
return {};
Q_UNREACHABLE_RETURN({});
}
#endif

6 changes: 2 additions & 4 deletions src/corelib/io/qurl.cpp
Original file line number Diff line number Diff line change
@@ -3495,8 +3495,7 @@ static QString errorMessage(QUrlPrivate::ErrorCode errorCode, const QString &err
case QUrlPrivate::NoError:
Q_ASSERT_X(false, "QUrl::errorString",
"Impossible: QUrl::errorString should have treated this condition");
Q_UNREACHABLE();
return QString();
Q_UNREACHABLE_RETURN(QString());

case QUrlPrivate::InvalidSchemeError: {
auto msg = "Invalid scheme (character '%1' not permitted)"_L1;
@@ -3554,8 +3553,7 @@ static QString errorMessage(QUrlPrivate::ErrorCode errorCode, const QString &err
}

Q_ASSERT_X(false, "QUrl::errorString", "Cannot happen, unknown error");
Q_UNREACHABLE();
return QString();
Q_UNREACHABLE_RETURN(QString());
}

static inline void appendComponentIfPresent(QString &msg, bool present, const char *componentName,
3 changes: 1 addition & 2 deletions src/corelib/kernel/qmetatype.cpp
Original file line number Diff line number Diff line change
@@ -2006,8 +2006,7 @@ static bool convertToEnum(QMetaType fromType, const void *from, QMetaType toType
*static_cast<qint64 *>(to) = value;
return true;
default:
Q_UNREACHABLE();
return false;
Q_UNREACHABLE_RETURN(false);
}
}

3 changes: 1 addition & 2 deletions src/corelib/kernel/qpropertyprivate.h
Original file line number Diff line number Diff line change
@@ -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<Callable *>(f)->~Callable(); },
6 changes: 2 additions & 4 deletions src/corelib/kernel/qvariant.cpp
Original file line number Diff line number Diff line change
@@ -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<int> integralCompare(uint promotedType, const QVariant::Pri
if (promotedType == QMetaType::ULongLong)
return spaceShip<qulonglong>(l1, l2);

Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

static std::optional<int> numericCompare(const QVariant::Private *d1, const QVariant::Private *d2)
6 changes: 2 additions & 4 deletions src/corelib/serialization/qcborstreamreader.cpp
Original file line number Diff line number Diff line change
@@ -36,13 +36,11 @@ QT_WARNING_POP

static CborError _cbor_value_dup_string(const CborValue *, void **, size_t *, CborValue *)
{
Q_UNREACHABLE();
return CborErrorInternalError;
Q_UNREACHABLE_RETURN(CborErrorInternalError);
}
[[maybe_unused]] static CborError cbor_value_get_half_float_as_float(const CborValue *, float *)
{
Q_UNREACHABLE();
return CborErrorInternalError;
Q_UNREACHABLE_RETURN(CborErrorInternalError);
}

// confirm our constants match TinyCBOR's
6 changes: 2 additions & 4 deletions src/corelib/serialization/qcborstreamwriter.cpp
Original file line number Diff line number Diff line change
@@ -29,14 +29,12 @@ QT_WARNING_POP
// but never defined
[[maybe_unused]] static CborError cbor_encoder_close_container_checked(CborEncoder*, const CborEncoder*)
{
Q_UNREACHABLE();
return CborErrorInternalError;
Q_UNREACHABLE_RETURN(CborErrorInternalError);
}

[[maybe_unused]] static CborError cbor_encode_float_as_half_float(CborEncoder *, float)
{
Q_UNREACHABLE();
return CborErrorInternalError;
Q_UNREACHABLE_RETURN(CborErrorInternalError);
}

Q_DECLARE_TYPEINFO(CborEncoder, Q_PRIMITIVE_TYPE);
3 changes: 1 addition & 2 deletions src/corelib/serialization/qjsoncbor.cpp
Original file line number Diff line number Diff line change
@@ -576,8 +576,7 @@ QVariant QCborValue::toVariant() const
if (isSimpleType())
return QVariant::fromValue(toSimpleType());

Q_UNREACHABLE();
return QVariant();
Q_UNREACHABLE_RETURN(QVariant());
}

/*!
3 changes: 1 addition & 2 deletions src/corelib/serialization/qjsonvalue.cpp
Original file line number Diff line number Diff line change
@@ -1097,8 +1097,7 @@ size_t qHash(const QJsonValue &value, size_t seed)
case QJsonValue::Undefined:
return seed;
}
Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

#if !defined(QT_NO_DEBUG_STREAM) && !defined(QT_JSON_READONLY)
6 changes: 2 additions & 4 deletions src/corelib/text/qbytearray.cpp
Original file line number Diff line number Diff line change
@@ -541,8 +541,7 @@ static const char *zlibOpAsString(ZLibOp op)
case ZLibOp::Compression: return "qCompress";
case ZLibOp::Decompression: return "qUncompress";
}
Q_UNREACHABLE();
return nullptr;
Q_UNREACHABLE_RETURN(nullptr);
}

Q_DECL_COLD_FUNCTION
@@ -2908,8 +2907,7 @@ QByteArray QByteArray::mid(qsizetype pos, qsizetype len) const
case QContainerImplHelper::Subset:
return QByteArray(d.data() + p, l);
}
Q_UNREACHABLE();
return QByteArray();
Q_UNREACHABLE_RETURN(QByteArray());
}

/*!
3 changes: 1 addition & 2 deletions src/corelib/text/qlocale_tools.cpp
Original file line number Diff line number Diff line change
@@ -570,8 +570,7 @@ QString qulltoa(qulonglong number, int base, const QStringView zero)
number /= base;
}
} else { // zero should always be either a non-surrogate or a surrogate pair:
Q_UNREACHABLE();
return QString();
Q_UNREACHABLE_RETURN(QString());
}

return QString(reinterpret_cast<QChar *>(p), end - p);
3 changes: 1 addition & 2 deletions src/corelib/text/qstring.cpp
Original file line number Diff line number Diff line change
@@ -5025,8 +5025,7 @@ QString QString::mid(qsizetype position, qsizetype n) const
case QContainerImplHelper::Subset:
return QString(constData() + p, l);
}
Q_UNREACHABLE();
return QString();
Q_UNREACHABLE_RETURN(QString());
}

/*!
3 changes: 1 addition & 2 deletions src/corelib/text/qstringalgorithms_p.h
Original file line number Diff line number Diff line change
@@ -47,8 +47,7 @@ template <typename StringType> struct QStringAlgorithms
static inline StringType trimmed_helper_inplace(const NakedStringType &, const Char *, const Char *)
{
// can't happen
Q_UNREACHABLE();
return StringType();
Q_UNREACHABLE_RETURN(StringType());
}

static inline void trimmed_helper_positions(const Char *&begin, const Char *&end)
2 changes: 1 addition & 1 deletion src/corelib/thread/qfutex_p.h
Original file line number Diff line number Diff line change
@@ -24,7 +24,7 @@ namespace QtDummyFutex {
constexpr inline bool futexAvailable() { return false; }
template <typename Atomic>
inline bool futexWait(Atomic &, typename Atomic::Type, int = 0)
{ Q_UNREACHABLE(); return false; }
{ Q_UNREACHABLE_RETURN(false); }
template <typename Atomic> inline void futexWakeOne(Atomic &)
{ Q_UNREACHABLE(); }
template <typename Atomic> inline void futexWakeAll(Atomic &)
6 changes: 2 additions & 4 deletions src/corelib/time/qdatetime.cpp
Original file line number Diff line number Diff line change
@@ -2841,8 +2841,7 @@ static inline bool usesSameOffset(const QDateTimeData &a, const QDateTimeData &b
Q_ASSERT(!a.isShort() && !b.isShort());
return a->m_offsetFromUtc == b->m_offsetFromUtc;
}
Q_UNREACHABLE();
return false;
Q_UNREACHABLE_RETURN(false);
}

// Refresh the LocalTime or TimeZone validity and offset
@@ -3805,8 +3804,7 @@ qint64 QDateTime::toMSecsSinceEpoch() const
#endif
return 0;
}
Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

/*!
6 changes: 2 additions & 4 deletions src/corelib/time/qdatetimeparser.cpp
Original file line number Diff line number Diff line change
@@ -1441,8 +1441,7 @@ QDateTimeParser::scanString(const QDateTime &defaultValue, bool fixup) const
// Don't care about date or spec, so pick a safe spec:
return StateNode(QDateTime(date, time, Qt::UTC), state, padding, conflicts);
default:
Q_UNREACHABLE();
return StateNode();
Q_UNREACHABLE_RETURN(StateNode());
}
}

@@ -2253,8 +2252,7 @@ QString QDateTimeParser::getAmPmText(AmPm ap, Case cs) const
case LowerCase: return raw.toLower();
case NativeCase: return raw;
}
Q_UNREACHABLE();
return raw;
Q_UNREACHABLE_RETURN(raw);
}

/*
1 change: 1 addition & 0 deletions src/corelib/tools/qatomicscopedvaluerollback_p.h
Original file line number Diff line number Diff line change
@@ -43,6 +43,7 @@ class [[nodiscard]] QAtomicScopedValueRollback
}
// GCC 8.x does not tread __builtin_unreachable() as constexpr
#if !defined(Q_CC_GNU_ONLY) || (Q_CC_GNU >= 900)
// NOLINTNEXTLINE(qt-use-unreachable-return): Triggers on Clang, breaking GCC 8
Q_UNREACHABLE();
#endif
return std::memory_order_seq_cst;
3 changes: 1 addition & 2 deletions src/corelib/tools/qfreelist_p.h
Original file line number Diff line number Diff line change
@@ -124,8 +124,7 @@ class QFreeList
return i;
x -= size;
}
Q_UNREACHABLE();
return -1;
Q_UNREACHABLE_RETURN(-1);
}

// allocate a block of the given \a size, initialized starting with the given \a offset
1 change: 1 addition & 0 deletions src/corelib/tools/qhash.cpp
Original file line number Diff line number Diff line change
@@ -107,6 +107,7 @@ struct HashSeedStorage
StateResult result = { 0, OverriddenByEnvironment };
#ifdef QT_BOOTSTRAPPED
Q_UNUSED(which);
// NOLINTNEXTLINE(qt-use-unreachable-return): triggers on QT_BOOTSTRAPPED, breaking #else case
Q_UNREACHABLE();
#else
// can't use qEnvironmentVariableIntValue (reentrancy)
3 changes: 1 addition & 2 deletions src/gui/image/qimage_conversions.cpp
Original file line number Diff line number Diff line change
@@ -1033,8 +1033,7 @@ static inline uint qUnpremultiplyRgb30(uint rgb30)
case 3:
return rgb30;
}
Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

template<bool rgbswap>
3 changes: 1 addition & 2 deletions src/gui/kernel/qshortcutmap.cpp
Original file line number Diff line number Diff line change
@@ -313,8 +313,7 @@ bool QShortcutMap::tryShortcut(QKeyEvent *e)
return identicalMatches > 0;
}
}
Q_UNREACHABLE();
return false;
Q_UNREACHABLE_RETURN(false);
}

/*! \internal
6 changes: 2 additions & 4 deletions src/gui/painting/qdrawhelper.cpp
Original file line number Diff line number Diff line change
@@ -50,8 +50,7 @@ constexpr int half_point = 1 << 15;
template <QPixelLayout::BPP bpp> static
inline uint QT_FASTCALL fetch1Pixel(const uchar *, int)
{
Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

template <>
@@ -4917,8 +4916,7 @@ void qBlendTexture(int count, const QSpan *spans, void *userData)
ProcessSpans proc;
switch (data->rasterBuffer->format) {
case QImage::Format_Invalid:
Q_UNREACHABLE();
return;
Q_UNREACHABLE_RETURN();
case QImage::Format_ARGB32_Premultiplied:
proc = processTextureSpansARGB32PM[blendType];
break;
3 changes: 1 addition & 2 deletions src/gui/painting/qpixellayout.cpp
Original file line number Diff line number Diff line change
@@ -212,8 +212,7 @@ inline void QT_FASTCALL storePixel<QPixelLayout::BPP24>(uchar *dest, int index,
template <QPixelLayout::BPP bpp> static
inline uint QT_FASTCALL fetchPixel(const uchar *, int)
{
Q_UNREACHABLE();
return 0;
Q_UNREACHABLE_RETURN(0);
}

template <>
3 changes: 1 addition & 2 deletions src/gui/painting/webgradients.cpp
Original file line number Diff line number Diff line change
@@ -346,8 +346,7 @@ static QList<QGradientStop> qt_preset_gradient_stops(QGradient::Preset preset)
case QGradient::NumPresets:
Q_UNREACHABLE();
}
Q_UNREACHABLE();
return {};
Q_UNREACHABLE_RETURN({});
}

static constexpr QGradient::QGradientData qt_preset_gradient_data[] = {
Loading
Oops, something went wrong.

0 comments on commit fc76767

Please sign in to comment.