[PATCH] D15531: Change linkInModule to take a std::unique_ptr

[whitequark via llvm-commits]

On 2015-12-15 19:15, Rafael Espíndola wrote:
> On 15 December 2015 at 11:07, whitequark <whitequark at whitequark.org> 
> wrote:
>> whitequark added inline comments.
>> 
>> ================
>> Comment at: test/Bindings/OCaml/linker.ml:58
>> @@ -57,3 +57,1 @@
>> -    dispose_module m1;
>> -    dispose_module m2
>> 
>> ----------------
>> This is really terrible; this is a silent change that alters 
>> correctness of existing OCaml code, leading to double free, which will 
>> not be detected if you don't run a Debug build of LLVM (and even then 
>> I'm not sure).
>> 
>> Could you please rename link_modules function to link_modules' (note 
>> the apostrophe) and add a bit to documentation in the mli like you did 
>> in the C API? It is probably not necessary to keep the old one for 
>> compatibility.
> 
> I can do it, but it seems a departure of what was previously done. My
> understanding is that only the C api has any form of backwards
> compatibility.

True, but the issue here is not backward compatibility (I am suggesting 
to break it!)
but the fact that OCaml programmers have a general expectation of memory 
safety.
It is bad enough that the LLVM bindings break it in several subtle ways,
but it will be *much* worse if existing code that used to be memory-safe
still compiles against new bindings but now isn't. This warrants special 
treatment.

-- 
whitequark