Clash between QByteArrayView and TString's operator==()

pmanek · June 1, 2024, 1:21pm

Hello, I think I discovered a bug in the latest ROOT version under C++20.

Symptoms to reproduce are as follows:

Create a C++20 project that depends on both ROOT and Qt.
Include TROOT.h and QObject in a header file.
Declare a struct in the same header and annotate it using the QT_DECLARE_METATYPE macro.

Expected behavior: the project compiles.

Actual behavior: compiation fails with the following error message:

/tmp/foo.h:378:1: error: use of overloaded operator '==' is ambiguous (with operand types 'QByteArrayView' and 'const char[13]')
  378 |   Q_DECLARE_METATYPE(foo)                                                             
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:1504:34: note: expanded from macro 'Q_DECLARE_METATYPE'
 1504 | #define Q_DECLARE_METATYPE(TYPE) Q_DECLARE_METATYPE_IMPL(TYPE)
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:1519:42: note: expanded from macro 'Q_DECLARE_METATYPE_IMPL'
 1519 |                 if (QByteArrayView(name) == (#TYPE)) {                  \
      |                     ~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~
/usr/include/qt6/QtCore/qbytearrayview.h:318:24: note: candidate function
  318 |     friend inline bool operator==(QByteArrayView lhs, QByteArrayView rhs) noexcept
      |                        ^
/usr/include/TString.h:856:15: note: candidate function
  856 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |               ^
/usr/include/TString.h:851:15: note: candidate function (with reversed parameter order)
  851 | inline Bool_t operator==(const char *s1, const std::string_view &s2)

It seems that when both Qt and ROOT are included in the same compilation unit, operator==(const char*, const std::string_view&) competes with operator==(QByteArrayView, QByteArrayView). Unfortunately for me as a user, I cannot really modify ROOT’s or Qt’s header files. My only mitigation is that I have to be extraordinarily careful about what ROOT headers I include in my Qt app.

I intend to report this also to Qt folks as this is primarily their bug, but since they are not exactly known for expediency, it would be super cool if TString.h could somehow detect and prevent ambiguity from occurring. Or perhaps respond to a pre-processor macro?

Any ideas/suggestions would be welcome.

ROOT Version: 6.30/06
Platform: Linux x86_64
Compiler: clang version 17.0.6, x86_64-pc-linux-gnu

Danilo · June 1, 2024, 1:50pm

Dear Petr,

Nice catch: thanks for reporting. I am adding in the loop @bellenot .

Best,
Danilo

bellenot · June 3, 2024, 9:31am

I really don’t know how we could fix that in TString.h, since we use regular types (i.e. std::string_view and const char *). Maybe @pcanal has a suggestion we could try?

pmanek · June 3, 2024, 10:15am

This is the offending function from qmetaobject.h:1512 (Qt v6.7.1). Note that the following snippet is defined inside a pre-processor macro, hence trailing backslashes.

            static int qt_metatype_id()                                 \
            {                                                           \
                Q_CONSTINIT static QBasicAtomicInt metatype_id = Q_BASIC_ATOMIC_INITIALIZER(0); \
                if (const int id = metatype_id.loadAcquire())           \
                    return id;                                          \
                constexpr auto arr = QtPrivate::typenameHelper<TYPE>(); \
                auto name = arr.data();                                 \
                if (QByteArrayView(name) == (#TYPE)) {                  \
                    const int id = qRegisterNormalizedMetaType<TYPE>(name); \
                    metatype_id.storeRelease(id);                       \
                    return id;                                          \
                }                                                       \
                const int newId = qRegisterMetaType< TYPE >(#TYPE);     \
                metatype_id.storeRelease(newId);                        \
                return newId;                                           \
            }

The token TYPE describes a name of a type, which is being registered in the Qt meta-object system, so #TYPE translates to a C string literal that is compared to a QByteArrayView. On the other end, TString.h:850 (ROOT 6.30.06) implements the following operators:

// To avoid ambiguities.
inline Bool_t operator==(const char *s1, const std::string_view &s2)
{
  return std::string_view(s1) == s2;
}

inline Bool_t operator==(const std::string_view &s1, const char *s2)
{
  return s1 == std::string_view(s2);
}

These appear to be the main source of ambiguity, as the compiler is not able to decide whether it should:

Convert #TYPE (C string literal) to QByteArrayView and then run Qt’s comparison operator for 2 instances of QByteArrayView, which I am assuming was the intended behavior.
Convert #TYPE to std::string_view using an implicit conversion operator from the STL and compare that with QByteArrayView
Leave #TYPE as is, convert QByteArrayView to std::string_view using an implicit conversion operator and run ROOT’s comparison operator.

Unless there is a very good reason for having them around, my instinct is telling me that the pair of comparison operators from TString.h should probably not be included in ROOT headers, as neither of the types they extend is defined by ROOT. Right now, simply including TString.h into a project seems to alter behavior of STL strings, string views and their comparison, which could be a recipe for undefined behavior later on.

The comparison operators in TString.h are currently disabled on Windows, so ROOT can clearly compile without them. The comment just above says: “to avoid ambiguities.” Can anyone clarify the type of ambiguities that motivated them in the first place?

If you look at std::string_view operators, you will find that quite a significant set of changes was implemented between C++17 and C++20, which may perhaps render the operators from TString.h unnecessary in C++20 projects. If that is the case, I could easily envision extending the pre-processor macro that currently disables them on Windows to perform an additional C++ standard check. Would it make sense to test that hypothesis?

bellenot · June 3, 2024, 10:47am

So it looks like the offending code in TString.h is gone in ROOT master. Can you try with master (or 6.32.00) to see if the problem is still there?
EDIT: And see in particular this commit

pmanek · June 3, 2024, 11:31am

So it looks like the offending code in TString.h is gone in ROOT master.

Nicely spotted. I was unaware of this.

Can you try with master (or 6.32.00) to see if the problem is still there?

Yes, sure. I will try and report back.

pmanek · June 9, 2024, 9:47am

Can you try with master (or 6.32.00) to see if the problem is still there?

So ROOT 6.32/00 finally reached my distro. After updating, I tried running my bug reproducer against it. I am pleased to report that the operator ambiguity issue no longer appears.

In other words: the latest ROOT version resolves my bug, as it enables me to #include <TString.h> along with Qt headers in a single compilation unit under C++20!

Thanks for your help and advice throughout this!

system · June 23, 2024, 9:48am

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.