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

[Artem Dergachev via Phabricator via llvm-commits]

NoQ added a comment.

In https://reviews.llvm.org/D49985#1181564, @dblaikie wrote:

> In https://reviews.llvm.org/D49985#1181018, @NoQ wrote:
>
> > I strongly support argument order swap because it's consistent with other immutable data structures.
>
>
> Not sure I follow - which examples of argument order are you comparing here?


`ImmutableMap` and `ImmutableSet` both have `add(Collection, Item)` argument order, but `ImmutableList` writes the same thing as `add(Item, Collection)`.

> It looks like concat orders the arguments in the same way that the output would be (so concat(X, list) produces [X, list]) - so preserving that argument order seems like it improves/retains readability compared to switching them around so 'concat(list, X)' produces '[X, list]'.

Yeah, i guess that might have been the motivation behind such inconsistency. I'll be fine with fixing the order for other data structures.

In https://reviews.llvm.org/D49985#1181564, @dblaikie wrote:

> In https://reviews.llvm.org/D49985#1181018, @NoQ wrote:
>
> > In https://reviews.llvm.org/D49985#1180850, @dblaikie wrote:
> >
> > > Test coverage?
> > >  ...
> > >  & does changing these signatures (of concat and add) not break any existing
> > >  code?
> >
> >
> > This is used only in clang and there's https://reviews.llvm.org/D49986 and there should be no functional change here.
>
>
> There's a functional change as described by the patch description - this data structure is getting new features/functionality and that can be unit tested. Also, it's best to test it in LLVM so that it doesn't get broken by LLVM developers who may not be compiling/testing Clang.


Yup, that makes sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D49985