Qt
  1. Qt
  2. QTBUG-26877

Fix some of the warnings that we currently disable in qglobal.h (MSVC)

    Details

    • Type: Suggestion Suggestion
    • Status: Open
    • Priority: P3: Somewhat important P3: Somewhat important
    • Resolution: Unresolved
    • Affects Version/s: 4.8.2, 5.0.0 Beta 1, 5.8.0
    • Fix Version/s: None
    • Component/s: Core: Other

      Description

      Currently qglobal.h disables some MSVC warnings if QT_NO_WARNINGS is defined:

      #if !defined(QT_CC_WARNINGS)
      #  define QT_NO_WARNINGS
      #endif
      #if defined(QT_NO_WARNINGS)
      #  if defined(Q_CC_MSVC)
      #    pragma warning(disable: 4251) /* class 'A' needs to have dll interface for to be used by clients of class 'B'. */
      #    pragma warning(disable: 4244) /* 'conversion' conversion from 'type1' to 'type2', possible loss of data */
      #    pragma warning(disable: 4275) /* non - DLL-interface classkey 'identifier' used as base for DLL-interface classkey 'identifier' */
      #    pragma warning(disable: 4514) /* unreferenced inline/local function has been removed */
      
      ...
      
      

      Defining QT_CC_WARNINGS is no real option, because i get tons of warnings for Qt code, if QT_CC_WARNINGS is defined and i am then unable to find the relevant places in my own code.

      Is there a work-around for this?

      If not then the Qt source code should be fixed such that it is not necessary to disable those warnings, because disabling those warnings in a vital header file like qglobal.h (which is included in many many other header files of Qt) does effect the code of the library user, which is not acceptable in security and safety critical applications (like those that we do develop).

      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        Sergey Semushin added a comment -

        >Vladimir Yelnikov
        On one hand that's a fine way but it's a bit hard to maintain, especially if you're compiling on different platforms, so I'm not sure if we would implement it in our code.

        By the way I checked warning 4244 (about narrowing conversion) which interests me, by including QtCore, QtNetwork, QtWidgets, QtPrintSupport, QtGui to single file and enabling this warning beforehand.
        It gave me around 20 warnings in 5-6 headers, I mean are really needed conversions couldn't be added and warning disabling removed? Probably there's gonna be something needed fixing in the rest of Qt headers also but it doesn't look like it's a lot. Don't have much information about other disabled warnings but this one seems to be really bug-finding sometimes.

        Of course the best thing would be if msvc would support something like -isystem in gcc but it doesn't look like there's any hope.

        Show
        Sergey Semushin added a comment - >Vladimir Yelnikov On one hand that's a fine way but it's a bit hard to maintain, especially if you're compiling on different platforms, so I'm not sure if we would implement it in our code. By the way I checked warning 4244 (about narrowing conversion) which interests me, by including QtCore, QtNetwork, QtWidgets, QtPrintSupport, QtGui to single file and enabling this warning beforehand. It gave me around 20 warnings in 5-6 headers, I mean are really needed conversions couldn't be added and warning disabling removed? Probably there's gonna be something needed fixing in the rest of Qt headers also but it doesn't look like it's a lot. Don't have much information about other disabled warnings but this one seems to be really bug-finding sometimes. Of course the best thing would be if msvc would support something like -isystem in gcc but it doesn't look like there's any hope.
        Hide
        Nicholas Chapman added a comment -

        Hi,
        We ran into this problem recently. In my opinion this is a somewhat serious problem with Qt that should be fixed.

        We noticed this after fixing a bug in our code. A function was incorrectly taking an int argument instead of a double argument, and we were passing a double in. But the usual warning was not reported The usual warning would be something like:

        warning C4244: 'initializing' : conversion from 'double' to 'int', possible loss of data

        After a little experimentation I determined that the Qt headers are effectively suppressing warnings for all code after they are included, due to the use of #pragma warning in qglobal.h as noted in this bug report.

        I don't think it's a good idea for Qt to disable warnings like this such that it effects user code. If Qt wasn't doing this we would have probably found our bug much earlier.

        One possible solution is to use #pragma warning push at the start of qt headers, then #pragma warning pop at the end.

        In my opinion, 'polluting' the compiler warning state in the way qt does is bad form, and is similar to 'using namespace std' in a header file, which we all know is a bad idea.

        Although there is a QT_NO_WARNINGS option, I think it's a good idea to try fixing with the pragma pop idea above. Having an option is better than nothing, but Qt should not have dodgy behaviour by default.

        Thanks,
        Nick C.

        Show
        Nicholas Chapman added a comment - Hi, We ran into this problem recently. In my opinion this is a somewhat serious problem with Qt that should be fixed. We noticed this after fixing a bug in our code. A function was incorrectly taking an int argument instead of a double argument, and we were passing a double in. But the usual warning was not reported The usual warning would be something like: warning C4244: 'initializing' : conversion from 'double' to 'int', possible loss of data After a little experimentation I determined that the Qt headers are effectively suppressing warnings for all code after they are included, due to the use of #pragma warning in qglobal.h as noted in this bug report. I don't think it's a good idea for Qt to disable warnings like this such that it effects user code. If Qt wasn't doing this we would have probably found our bug much earlier. One possible solution is to use #pragma warning push at the start of qt headers, then #pragma warning pop at the end. In my opinion, 'polluting' the compiler warning state in the way qt does is bad form, and is similar to 'using namespace std' in a header file, which we all know is a bad idea. Although there is a QT_NO_WARNINGS option, I think it's a good idea to try fixing with the pragma pop idea above. Having an option is better than nothing, but Qt should not have dodgy behaviour by default. Thanks, Nick C.
        Hide
        Thiago Macieira added a comment -

        Damned if you do, damned if you don't...

        Nicholas Chapman, I would tend to agree with you that Qt should not disable warnings on a whim. The warnings that you enable in the compiler settings should be allowed to apply to all code. It's worth to note that those #pragmas have been there for a long time, prior to Qt's current policy of disabling warnings only surrounding code that it really affects.

        However, we've tried to disable the #pragmas in the past and the result was that people complained that Qt headers produce too much noise. Moreover, some things cannot be fixed, as the warnings are really false positives and, in my opinion, compiler braindamage, like 4251, 4275 and 4514. I've also resisted change that makes the Qt headers noisy. Those cast warnings for signedness change are a good example.

        So here's what I am willing to accept:

        • fix Qt code that triggers legitimate warnings. For example, cast from double to integrals should be explicit.
        • add some workaround to false-positive Qt warnings, provided that they don't make the code too noisy. This will be reviewed on a case-by-case basis
        • the rest of the warnings that I consider braindamage will remain disabled, unless QT_CC_WARNINGS is defined.

        Furthermore, note I will not do this work. I will accept patches from people who volunteer to do it, but I will not do it myself. Fixing warnings that are already disabled on MSVC has absolutely no priority to me.

        Show
        Thiago Macieira added a comment - Damned if you do, damned if you don't... Nicholas Chapman , I would tend to agree with you that Qt should not disable warnings on a whim. The warnings that you enable in the compiler settings should be allowed to apply to all code. It's worth to note that those #pragmas have been there for a long time, prior to Qt's current policy of disabling warnings only surrounding code that it really affects. However, we've tried to disable the #pragmas in the past and the result was that people complained that Qt headers produce too much noise. Moreover, some things cannot be fixed, as the warnings are really false positives and, in my opinion, compiler braindamage, like 4251, 4275 and 4514. I've also resisted change that makes the Qt headers noisy. Those cast warnings for signedness change are a good example. So here's what I am willing to accept: fix Qt code that triggers legitimate warnings. For example, cast from double to integrals should be explicit. add some workaround to false-positive Qt warnings, provided that they don't make the code too noisy. This will be reviewed on a case-by-case basis the rest of the warnings that I consider braindamage will remain disabled, unless QT_CC_WARNINGS is defined. Furthermore, note I will not do this work. I will accept patches from people who volunteer to do it, but I will not do it myself. Fixing warnings that are already disabled on MSVC has absolutely no priority to me.
        Hide
        Nicholas Chapman added a comment -

        Hi Thiago,
        Thanks for the reply.
        I would say the best, most correct way of fixing this issue is to surround Qt code with #pragma warning( push ) and #pragma warning( pop ). The pragma warning push would go near the top of the Qt header file, and the pop near the bottom.
        In this way Qt headers can suppress any warnings they want, and it won't affect other user code.
        I have noticed that some Qt headers actually do this already, such as qhash.h in Qt 5.5.1.

        Show
        Nicholas Chapman added a comment - Hi Thiago, Thanks for the reply. I would say the best, most correct way of fixing this issue is to surround Qt code with #pragma warning( push ) and #pragma warning( pop ). The pragma warning push would go near the top of the Qt header file, and the pop near the bottom. In this way Qt headers can suppress any warnings they want, and it won't affect other user code. I have noticed that some Qt headers actually do this already, such as qhash.h in Qt 5.5.1.
        Hide
        Kim added a comment -

        The most correct way is fixing the problem imo.

        template <typename T>
        int QVector<T>::indexOf(const T &t, int from) const
        {
            if (from < 0)
                from = qMax(from + d->size, 0);
            if (from < d->size) {
                T* n = d->begin() + from - 1;
                T* e = d->end();
                while (++n != e)
                    if (*n == t)
                        return n - d->begin();
            }
            return -1;
        }
        

        Here "return n - d->begin();" returns a 64bit value on 64bit systems, so the warning is correct...

        Show
        Kim added a comment - The most correct way is fixing the problem imo. template <typename T> int QVector<T>::indexOf( const T &t, int from) const { if (from < 0) from = qMax(from + d->size, 0); if (from < d->size) { T* n = d->begin() + from - 1; T* e = d->end(); while (++n != e) if (*n == t) return n - d->begin(); } return -1; } Here "return n - d->begin();" returns a 64bit value on 64bit systems, so the warning is correct...

          People

          • Assignee:
            Unassigned
            Reporter:
            Qt Support
          • Votes:
            5 Vote for this issue
            Watchers:
            13 Start watching this issue

            Dates

            • Created:
              Updated:

              Gerrit Reviews

              There are no open Gerrit changes