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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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?

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

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

> Not sure why didn't we ever write unit tests for these data structures. But it should more or less work as tested by clang lit tests.

Yeah, often the older corners don't have unit tests - but when they're improved/changed, it's a good chance to correct that & start putting in unit tests. The overhead/work involved in making the first unit test isn't high enough that I feel like it's a great imposition to ask the next developer to start it.


Repository:
  rL LLVM

https://reviews.llvm.org/D49985