<div dir="ltr">Test coverage?<br><br>(& also, maybe rather than modifying concat's signature (& causing its arguments to reverse in a way that might be confusing), just take the argument by value and move into the destination?)<br><br>& does changing these signatures (of concat and add) not break any existing code?</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 7:03 AM Umann Kristóf via Phabricator via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Szelethus created this revision.<br>
Szelethus added reviewers: NoQ, george.karpenkov, dcoughlin, chandlerc.<br>
Herald added subscribers: llvm-commits, dexonsmith.<br>
<br>
I'm refactoring my Static Analyzer checker `UninitializedObjectChecker`, and I need to store polymorphic objects in `llvm::ImmutableList`. I wish to store `std::unique_ptr`s, but `llvm::ImmutableList` requires it's elements to be copy constructible for no good reason. This patch aims to fix this.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D49985" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985</a><br>
<br>
Files:<br>
  include/llvm/ADT/ImmutableList.h<br>
<br>
<br>
Index: include/llvm/ADT/ImmutableList.h<br>
===================================================================<br>
--- include/llvm/ADT/ImmutableList.h<br>
+++ include/llvm/ADT/ImmutableList.h<br>
@@ -31,8 +31,8 @@<br>
   T Head;<br>
   const ImmutableListImpl* Tail;<br>
<br>
-  ImmutableListImpl(const T& head, const ImmutableListImpl* tail = nullptr)<br>
-    : Head(head), Tail(tail) {}<br>
+  ImmutableListImpl(T&& head, const ImmutableListImpl* tail = nullptr)<br>
+    : Head(std::move(head)), Tail(tail) {}<br>
<br>
 public:<br>
   ImmutableListImpl(const ImmutableListImpl &) = delete;<br>
@@ -166,38 +166,42 @@<br>
     if (ownsAllocator()) delete &getAllocator();<br>
   }<br>
<br>
-  LLVM_NODISCARD ImmutableList<T> concat(const T &Head, ImmutableList<T> Tail) {<br>
+  template <typename ...CtorArgs><br>
+  LLVM_NODISCARD ImmutableList<T> concat(ImmutableList<T> Tail, CtorArgs &&...Args) {<br>
     // Profile the new list to see if it already exists in our cache.<br>
     FoldingSetNodeID ID;<br>
     void* InsertPos;<br>
<br>
     const ListTy* TailImpl = Tail.getInternalPointer();<br>
+    T Head(std::forward<CtorArgs>(Args)...);<br>
     ListTy::Profile(ID, Head, TailImpl);<br>
     ListTy* L = Cache.FindNodeOrInsertPos(ID, InsertPos);<br>
<br>
     if (!L) {<br>
       // The list does not exist in our cache.  Create it.<br>
       BumpPtrAllocator& A = getAllocator();<br>
       L = (ListTy*) A.Allocate<ListTy>();<br>
-      new (L) ListTy(Head, TailImpl);<br>
+      new (L) ListTy(std::move(Head), TailImpl);<br>
<br>
       // Insert the new list into the cache.<br>
       Cache.InsertNode(L, InsertPos);<br>
     }<br>
<br>
     return L;<br>
   }<br>
<br>
-  LLVM_NODISCARD ImmutableList<T> add(const T& D, ImmutableList<T> L) {<br>
-    return concat(D, L);<br>
+  template <typename ...CtorArgs><br>
+  LLVM_NODISCARD ImmutableList<T> add(ImmutableList<T> Tail, CtorArgs &&...Args) {<br>
+    return concat(Tail, std::forward<CtorArgs>(Args)...);<br>
   }<br>
<br>
   ImmutableList<T> getEmptyList() const {<br>
     return ImmutableList<T>(nullptr);<br>
   }<br>
<br>
-  ImmutableList<T> create(const T& X) {<br>
-    return Concat(X, getEmptyList());<br>
+  template <typename ...CtorArgs><br>
+  ImmutableList<T> create(CtorArgs &&...Args) {<br>
+    return concat(getEmptyList(), std::forward<CtorArgs>(Args)...);<br>
   }<br>
 };<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>