<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 6:42 PM Artem Dergachev via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">NoQ added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D49985#1181564" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985#1181564</a>, @dblaikie wrote:<br>
<br>
> In <a href="https://reviews.llvm.org/D49985#1181018" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985#1181018</a>, @NoQ wrote:<br>
><br>
> > I strongly support argument order swap because it's consistent with other immutable data structures.<br>
><br>
><br>
> Not sure I follow - which examples of argument order are you comparing here?<br>
<br>
<br>
`ImmutableMap` and `ImmutableSet` both have `add(Collection, Item)` argument order, but `ImmutableList` writes the same thing as `add(Item, Collection)`.<br></blockquote><div><br></div><div>Ah, thanks!</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> 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]'.<br>
<br>
Yeah, i guess that might have been the motivation behind such inconsistency. I'll be fine with fixing the order for other data structures.<br></blockquote><div><br>Not sure there's a good consistency between data structures with these names/descriptions.<br><br>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)?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In <a href="https://reviews.llvm.org/D49985#1181564" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985#1181564</a>, @dblaikie wrote:<br>
<br>
> In <a href="https://reviews.llvm.org/D49985#1181018" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985#1181018</a>, @NoQ wrote:<br>
><br>
> > In <a href="https://reviews.llvm.org/D49985#1180850" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49985#1180850</a>, @dblaikie wrote:<br>
> ><br>
> > > Test coverage?<br>
> > >  ...<br>
> > >  & does changing these signatures (of concat and add) not break any existing<br>
> > >  code?<br>
> ><br>
> ><br>
> > This is used only in clang and there's <a href="https://reviews.llvm.org/D49986" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49986</a> and there should be no functional change here.<br>
><br>
><br>
> 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.<br>
<br>
<br>
Yup, that makes sense.<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>
<br>
<br>
</blockquote></div></div>