Merge r310475 into the 5.0 branch : [Support] #33388 - Fix formatv_object move constructor

[llvmbot]

Bugzilla Link	34987
Resolution	FIXED
Resolved on	Nov 15, 2017 12:49
Version	trunk
OS	All
Blocks	#33840
Reporter	LLVM Bugzilla Contributor
CC	@adsk-belleyb
Fixed by commit(s)	r310475 r318333

Extended Description

Is it OK to merge the following revision(s) to the 5.0 branch?

[llvmbot]

https://reviews.llvm.org/rL310475

[tstellar]

Will deleting:

formatv_object_base(formatv_object_base const &rhs) = delete;

Break the ABI?

[adsk-belleyb]

AFAIK, the change maintains both ABI and API compatibility.

The ABI compatibility is maintained because there was no user-defined copy constructor, only an implicitly defined one. A quick listing of the symbols of libLLVMSupport.a of LLVM 5.0.0 confirms that it isn't exporting any symbols for the formatv_object_base constructors (they are all either inlined or implicitly declared).

The API compatibility is also maintained (at least in a practical sense). No one could have explicitly invoked the implicit copy constructor without crashing because it was buggy. Thus, I believe that formatv_object has ever only been used in situations where the calls to the copy constructor were elided (like when returning a formatv_object by value). With the proposed change, the compiler will simply use the move constructor instead (and potentially elided it also!).

In theory, one could detect the change through things like std::is_copy_constructible<formatv_object_base>, but I can't see how that would lead to introducing regressions in useful code.

[tstellar]

Hi Zach,

You reviewed and committed this patch, is it OK for the release_50 branch?

https://reviews.llvm.org/rL310475

[llvmbot]

Agree with Comment #​3, it should be fine.

[tstellar]

Merged: r318333