[PATCH] D13023: [Bug 21683 relative] - Refactor of SimpleDefinedAtom::sortReferences()

[Rui Ueyama via llvm-commits]

Is the indentation correct?

On Mon, Sep 21, 2015 at 8:07 AM, George Rimar <grimar at accesssoftek.com>
wrote:

> grimar created this revision.
> grimar added reviewers: ruiu, kledzik.
> grimar added subscribers: grimar, llvm-commits.
> grimar set the repository for this revision to rL LLVM.
> grimar added a project: lld.
>
> I spent some time on investigating
> https://llvm.org/bugs/show_bug.cgi?id=21683
> It looks already fixed in r223528 which switched SimpleDefinedAtom to
> allocate its SimpleReferences using the BumpPtrAllocator.
>
> The only thing that does not look good for me is that
> SimpleDefinedAtom::sortReferences()
> method uses temp SmallVector for copying into and sorting the ilist
> content and after that recreates the list again.
> Since r247978 introduced ilist::sort I suggest the patch to use it.
>
> Anyways I believe bug 21683 can be closed with this patch or without as
> fixed.
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13023
>
> Files:
>   Simple.h
>
> Index: Simple.h
> ===================================================================
> --- Simple.h
> +++ Simple.h
> @@ -175,10 +175,23 @@
>    }
>
>    lld::SimpleReference *createSentinel() const {
> +       // absence of allocator generally is not an issue. That might be
> useful for
> +       // ilists of SimpleReferences created as temporatily local objects.
> +       // They dont need to be allocated via BumpPtrAllocator
> +       if (!_allocator) {
> +               return new lld::SimpleReference();
> +       }
> +
>      return new (*_allocator) lld::SimpleReference();
>    }
>
> -  static void destroySentinel(lld::SimpleReference*) {}
> +  void destroySentinel(lld::SimpleReference *sentinel) {
> +       // the same situation as described above in createSentinel()
> +       // there is no BumpPtrAllocator allocator sometimes and its fine.
> +       if (!_allocator) {
> +               delete sentinel;
> +       }
> +  }
>

I'd allocate the sentinel without the allocator. This code leaks the
sentinel if you set an allocator after a sentinel is allocated without the
allocator.

   static lld::SimpleReference *provideInitialHead() { return nullptr; }
>
> @@ -267,28 +280,17 @@
>
>    /// Sort references in a canonical order (by offset, then by kind).
>    void sortReferences() const {
> -    // Cannot sort a linked  list, so move elements into a temporary
> vector,
> -    // sort the vector, then reconstruct the list.
> -    llvm::SmallVector<SimpleReference *, 16> elements;
> -    for (SimpleReference &node : _references) {
> -      elements.push_back(&node);
> -    }
> -    std::sort(elements.begin(), elements.end(),
> -        [] (const SimpleReference *lhs, const SimpleReference *rhs) ->
> bool {
> -          uint64_t lhsOffset = lhs->offsetInAtom();
> -          uint64_t rhsOffset = rhs->offsetInAtom();
> -          if (rhsOffset != lhsOffset)
> -            return (lhsOffset < rhsOffset);
> -          if (rhs->kindNamespace() != lhs->kindNamespace())
> -            return (lhs->kindNamespace() < rhs->kindNamespace());
> -          if (rhs->kindArch() != lhs->kindArch())
> -            return (lhs->kindArch() < rhs->kindArch());
> -          return (lhs->kindValue() < rhs->kindValue());
> -        });
> -    _references.clearAndLeakNodesUnsafely();
> -    for (SimpleReference *node : elements) {
> -      _references.push_back(node);
> -    }
> +       _references.sort([](SimpleReference &lhs, SimpleReference &rhs) ->
> bool {
> +               uint64_t lhsOffset = lhs.offsetInAtom();
> +               uint64_t rhsOffset = rhs.offsetInAtom();
> +               if (rhsOffset != lhsOffset)
> +                       return (lhsOffset < rhsOffset);
> +               if (rhs.kindNamespace() != lhs.kindNamespace())
> +                       return (lhs.kindNamespace() < rhs.kindNamespace());
> +               if (rhs.kindArch() != lhs.kindArch())
> +                       return (lhs.kindArch() < rhs.kindArch());
> +               return (lhs.kindValue() < rhs.kindValue());
> +       });
>    }
>    void setOrdinal(uint64_t ord) { _ordinal = ord; }
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150921/3267ab98/attachment.html>