Skip to content
  • Igor Kushnir's avatar
    Path::segments(): return a const reference instead of a copy · d8a7b7fc
    Igor Kushnir authored
    Don't copy Path segments unnecessarily in qHash(const Path&). This
    function may be called often in performance-critical code.
    
    This commit speeds up BenchQuickOpen::benchProjectFileFilter_setFilter
    by about 10% when the filter is not empty. matchPathFilter() calls
    Path::segments() twice, is called by PathFilter::setFilter() in a loop.
    
    I have examined all usages of Path::segments() in KDevelop code and
    found no possibility of a regression because of this return type change.
    There are no usages of Path::segments() in maintained KDevelop plugins.
    
    In many usages copying the QVector is unnecessary and it doesn't occur
    with this change, which improves performance. Copying a QVector could be
    the bottleneck in some cases. One such case was a lambda in
    ProjectFileDataProvider::projectClosing(), which called this function on
    each file's projectPath (in an optimization now discarded in favor of an
    almost equivalent Path::operator== change in the previous commit).
    
    Arguments for returning the QVector by value:
      * returning objects by value is generally considered safer than
        returning them by reference;
      * returning by value gives more freedom to change the implementation
        of a class without changing its interface;
      * non-template Qt classes conventionally return by value relying on
        copy-on-write to make this relatively cheap.
    
    Arguments for returning the QVector by reference:
      * Returning by reference is not very dangerous. This pattern is not
        uncommon in non-COW C++ code and is used extensively by Qt and std
        container class templates, so developers know what to expect and how
        to use such interfaces safely. If callers of this function store the
        return value in a reference variable for optimization, they are
        responsible for ensuring the safety of such usage. If they simply
        use segments() in an expression or assign its return value to a
        non-reference variable (e.g. with `auto`), then they should be safe
        from reference-related bugs.
      * Path is a class whose main purpose is optimization. Its layout is
        already inline. If it switches to some other class for storing its
        internal data, the ABI change is unavoidable; the not so many usages
        of segments() could be adjusted accordingly rather than become
        silently a lot less efficient.
      * Returning QVector by value not only imposes the cost of copying it,
        but creates a risk of detaching if not used carefully: even when a
        Path is constant, `Path::segments() const` returns a temporary,
        non-const QVector. So the users of this function should take care to
        call only purely const QVector member functions or wrap segments()
        in as_const or assign segments() to a temporary constant variable.
    d8a7b7fc