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

[David Blaikie via llvm-commits]

On Mon, Jul 30, 2018 at 6:42 PM Artem Dergachev via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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)`.
>

Ah, thanks!

> 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.
>

Not sure there's a good consistency between data structures with these
names/descriptions.

Might make sense for List to have completely separately named functions -
like maybe list should only have concat(T, list) and Map and Set can have
add(container, T)?


>
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180730/4fd4b198/attachment.html>