[PATCH] D49985: [ADT] ImmutableList no longer requires elements to be copy constructible
Artem Dergachev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 30 18:42:44 PDT 2018
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
More information about the llvm-commits
mailing list