Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-103948

QQmlJS::Dom should use std::make_shared

    XMLWordPrintable

Details

    • c67f129cc3 (qt/qtdeclarative/dev) c67f129cc3 (qt/tqtc-qtdeclarative/dev)

    Description

      The QMLDOM classes use std::shared_ptr (good! QSharedPointer shouldn't be used in new code anymore - it's twice as slow as std::shared_ptr), but created them using manual new and shared_ptr(X*):

      std::shared_ptr<T> p(new T(args...));
      

      We depend on C++17 by now, so the worst part of this anti-pattern has been fixed (evaluation order has been pinned down the new'ed up T isn't leaked when something throws), but this performs at least two memory allocations: the T (plus anything T allocates) and the control block for the shared_ptr ref-counts (and deleter).

      By contrast, std::make_shared will perform only one memory allocation, because, since it controls the creation of the payload object now, it can place the T and the control block into a single memory allocation. Prior to C++17, it was also safer in the face of exception.

      It's also less to type, considering that T's type needs to be named only once, so there's absolutely no reason to use shared_ptr(new T) over make_shared<T>(). The only exception is: if the T is large, and there are weak_ptr s, then one might use shared_ptr(new T) to free T's (direct) memory sooner. But such use would need to carry a code comment, which is absent, so I guess it doesn't apply in this case.

      Acceptance criteria:

      • all shared_ptr s are constructed with make_shared
      • those who aren't have gotten a code comment explaining why not

      Attachments

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

        Activity

          People

            lavf Leticia Valladares
            mmutz Marc Mutz
            Vladimir Minenko Vladimir Minenko
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes