I accidentally created a naming inconsistency in the OCaml API

I recently added a function called pointer_type_in_context to the OCaml API, which wrapped the C function LLVMPointerTypeInContext. However, after my patch was merged, I noticed that the corresponding OCaml function of the C function LLVMVoidTypeInContext was called void_type, the corresponding OCaml function of LLVMStructTypeInContext was called struct_type, and so on. No other OCaml function had the in_context suffix, even though they wrapped a C function ending with InContext. So, I made a mistake.

I was going to create a fix, but then I noticed that there was already a function called pointer_type, which corresponded with the C function LLVMPointerType. So, I would not even be able to follow the naming rule above for the binding I added.

pointer_type creates a non-opaque pointer, while the binding I added creates an opaque pointer, and LLVM is migrating to opaque pointers. So, I assume that the other function will eventually be deprecated and its name will be available for the newer function.

However, did I make a mistake in my patch, and does my binding need to be renamed?

There are number of functions in the OCaml API that have been added as part of the opaque pointers transition, and generally the names are just the existing ones with a 2 suffix. The functions dealing with non-opaque pointers will eventually be removed, see ⚙ D135271 [llvm-c] Remove C API functions that are incompatible with opaque pointers for one upcoming example. Perhaps at that point functions without the suffix will be added, or we will just use the numeric-suffixed names indefinitely, I don’t know.

But I guess that the current pattern could be followed by renaming pointer_type_in_context to pointer_type2.

1 Like

I will create a patch to make this change when I get home from work.

This depends on whether the OCaml bindings have some kind of stability requirement, where the signature of existing functions cannot change. As it is not an FFI API I’d guess the answer is “no” and it’s fine to change them, but I’m not sure on this.

If the OCaml API is not stable, then it would indeed make sense to drop the old typed pointer APIs, and drop the 2 suffix from the new ones. (Feel free to do that independently of the C API removal, I don’t think there is any reason to delay this for the OCaml bindings.)

1 Like

I now have two diffs: ⚙ D135499 [llvm-ocaml] Revise OCaml opaque pointer bindings to match rest of API and ⚙ D135524 [llvm-ocaml] Replace all typed pointer functions with opaque pointer functions. The first one fixes my naming mistake by replacing pointer_type_in_context with pointer_type2 and qualified_pointer_type2. The second one goes ahead and replaces all the functions from the typed pointer API with their opaque pointer equivalents, including fixing the pointer_type_in_context naming mistake.