[PATCH] D49985: [ADT] ImmutableList no longer requires elements to be copy constructible

[David Blaikie via llvm-commits]

Test coverage?

(& 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?)

& does changing these signatures (of concat and add) not break any existing
code?

On Mon, Jul 30, 2018 at 7:03 AM Umann Kristóf via Phabricator via
llvm-commits <llvm-commits at lists.llvm.org> wrote:

> Szelethus created this revision.
> Szelethus added reviewers: NoQ, george.karpenkov, dcoughlin, chandlerc.
> Herald added subscribers: llvm-commits, dexonsmith.
>
> 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.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49985
>
> Files:
>   include/llvm/ADT/ImmutableList.h
>
>
> Index: include/llvm/ADT/ImmutableList.h
> ===================================================================
> --- include/llvm/ADT/ImmutableList.h
> +++ include/llvm/ADT/ImmutableList.h
> @@ -31,8 +31,8 @@
>    T Head;
>    const ImmutableListImpl* Tail;
>
> -  ImmutableListImpl(const T& head, const ImmutableListImpl* tail =
> nullptr)
> -    : Head(head), Tail(tail) {}
> +  ImmutableListImpl(T&& head, const ImmutableListImpl* tail = nullptr)
> +    : Head(std::move(head)), Tail(tail) {}
>
>  public:
>    ImmutableListImpl(const ImmutableListImpl &) = delete;
> @@ -166,38 +166,42 @@
>      if (ownsAllocator()) delete &getAllocator();
>    }
>
> -  LLVM_NODISCARD ImmutableList<T> concat(const T &Head, ImmutableList<T>
> Tail) {
> +  template <typename ...CtorArgs>
> +  LLVM_NODISCARD ImmutableList<T> concat(ImmutableList<T> Tail, CtorArgs
> &&...Args) {
>      // Profile the new list to see if it already exists in our cache.
>      FoldingSetNodeID ID;
>      void* InsertPos;
>
>      const ListTy* TailImpl = Tail.getInternalPointer();
> +    T Head(std::forward<CtorArgs>(Args)...);
>      ListTy::Profile(ID, Head, TailImpl);
>      ListTy* L = Cache.FindNodeOrInsertPos(ID, InsertPos);
>
>      if (!L) {
>        // The list does not exist in our cache.  Create it.
>        BumpPtrAllocator& A = getAllocator();
>        L = (ListTy*) A.Allocate<ListTy>();
> -      new (L) ListTy(Head, TailImpl);
> +      new (L) ListTy(std::move(Head), TailImpl);
>
>        // Insert the new list into the cache.
>        Cache.InsertNode(L, InsertPos);
>      }
>
>      return L;
>    }
>
> -  LLVM_NODISCARD ImmutableList<T> add(const T& D, ImmutableList<T> L) {
> -    return concat(D, L);
> +  template <typename ...CtorArgs>
> +  LLVM_NODISCARD ImmutableList<T> add(ImmutableList<T> Tail, CtorArgs
> &&...Args) {
> +    return concat(Tail, std::forward<CtorArgs>(Args)...);
>    }
>
>    ImmutableList<T> getEmptyList() const {
>      return ImmutableList<T>(nullptr);
>    }
>
> -  ImmutableList<T> create(const T& X) {
> -    return Concat(X, getEmptyList());
> +  template <typename ...CtorArgs>
> +  ImmutableList<T> create(CtorArgs &&...Args) {
> +    return concat(getEmptyList(), std::forward<CtorArgs>(Args)...);
>    }
>  };
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180730/cc09a685/attachment.html>