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

[George Rimar via llvm-commits]

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;
+	}
+  }
 
   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 --------------
A non-text attachment was scrubbed...
Name: D13023.35256.patch
Type: text/x-patch
Size: 2594 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150921/31629371/attachment.bin>