<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Is the indentation correct?</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Mon, Sep 21, 2015 at 8:07 AM, George Rimar <span dir="ltr"><<a href="mailto:grimar@accesssoftek.com" target="_blank">grimar@accesssoftek.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar created this revision.<br>
grimar added reviewers: ruiu, kledzik.<br>
grimar added subscribers: grimar, llvm-commits.<br>
grimar set the repository for this revision to rL LLVM.<br>
grimar added a project: lld.<br>
<br>
I spent some time on investigating <a href="https://llvm.org/bugs/show_bug.cgi?id=21683" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=21683</a><br>
It looks already fixed in r223528 which switched SimpleDefinedAtom to allocate its SimpleReferences using the BumpPtrAllocator.<br>
<br>
The only thing that does not look good for me is that SimpleDefinedAtom::sortReferences()<br>
method uses temp SmallVector for copying into and sorting the ilist content and after that recreates the list again.<br>
Since r247978 introduced ilist::sort I suggest the patch to use it.<br>
<br>
Anyways I believe bug 21683 can be closed with this patch or without as fixed.<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D13023" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13023</a><br>
<br>
Files:<br>
  Simple.h<br>
<br>
Index: Simple.h<br>
===================================================================<br>
--- Simple.h<br>
+++ Simple.h<br>
@@ -175,10 +175,23 @@<br>
   }<br>
<br>
   lld::SimpleReference *createSentinel() const {<br>
+       // absence of allocator generally is not an issue. That might be useful for<br>
+       // ilists of SimpleReferences created as temporatily local objects.<br>
+       // They dont need to be allocated via BumpPtrAllocator<br>
+       if (!_allocator) {<br>
+               return new lld::SimpleReference();<br>
+       }<br>
+<br>
     return new (*_allocator) lld::SimpleReference();<br>
   }<br>
<br>
-  static void destroySentinel(lld::SimpleReference*) {}<br>
+  void destroySentinel(lld::SimpleReference *sentinel) {<br>
+       // the same situation as described above in createSentinel()<br>
+       // there is no BumpPtrAllocator allocator sometimes and its fine.<br>
+       if (!_allocator) {<br>
+               delete sentinel;<br>
+       }<br>
+  }<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
   static lld::SimpleReference *provideInitialHead() { return nullptr; }<br>
<br>
@@ -267,28 +280,17 @@<br>
<br>
   /// Sort references in a canonical order (by offset, then by kind).<br>
   void sortReferences() const {<br>
-    // Cannot sort a linked  list, so move elements into a temporary vector,<br>
-    // sort the vector, then reconstruct the list.<br>
-    llvm::SmallVector<SimpleReference *, 16> elements;<br>
-    for (SimpleReference &node : _references) {<br>
-      elements.push_back(&node);<br>
-    }<br>
-    std::sort(elements.begin(), elements.end(),<br>
-        [] (const SimpleReference *lhs, const SimpleReference *rhs) -> bool {<br>
-          uint64_t lhsOffset = lhs->offsetInAtom();<br>
-          uint64_t rhsOffset = rhs->offsetInAtom();<br>
-          if (rhsOffset != lhsOffset)<br>
-            return (lhsOffset < rhsOffset);<br>
-          if (rhs->kindNamespace() != lhs->kindNamespace())<br>
-            return (lhs->kindNamespace() < rhs->kindNamespace());<br>
-          if (rhs->kindArch() != lhs->kindArch())<br>
-            return (lhs->kindArch() < rhs->kindArch());<br>
-          return (lhs->kindValue() < rhs->kindValue());<br>
-        });<br>
-    _references.clearAndLeakNodesUnsafely();<br>
-    for (SimpleReference *node : elements) {<br>
-      _references.push_back(node);<br>
-    }<br>
+       _references.sort([](SimpleReference &lhs, SimpleReference &rhs) -> bool {<br>
+               uint64_t lhsOffset = lhs.offsetInAtom();<br>
+               uint64_t rhsOffset = rhs.offsetInAtom();<br>
+               if (rhsOffset != lhsOffset)<br>
+                       return (lhsOffset < rhsOffset);<br>
+               if (rhs.kindNamespace() != lhs.kindNamespace())<br>
+                       return (lhs.kindNamespace() < rhs.kindNamespace());<br>
+               if (rhs.kindArch() != lhs.kindArch())<br>
+                       return (lhs.kindArch() < rhs.kindArch());<br>
+               return (lhs.kindValue() < rhs.kindValue());<br>
+       });<br>
   }<br>
   void setOrdinal(uint64_t ord) { _ordinal = ord; }<br>
<br>
<br>
<br>
</blockquote></div><br></div></div>