[PATCH] D18736: [llvm-c] Improve IR Introspection: Add LLVM{Get, Set}ModuleIdentifier

[Nicole Mazzuca via llvm-commits]

ubsan added inline comments.

================
Comment at: include/llvm-c/Core.h:483-496
@@ -482,2 +482,16 @@
 
 /**
+ * Obtain the identifier of a module. The result must be discarded with
+ * LLVMDisposeMessage.
+ *
+ * @see Module::getModuleIdentifier()
+ */
+char *LLVMGetModuleIdentifier(LLVMModuleRef M);
+
+/**
+ * Set the identifier of a module.
+ *
+ * @see Module::setModuleIdentifier()
+ */
+void LLVMSetModuleIdentifier(LLVMModuleRef M, const char *ID);
+
----------------
whitequark wrote:
> deadalnix wrote:
> > Wallbraker wrote:
> > > whitequark wrote:
> > > > Wallbraker wrote:
> > > > > Please add a length parameter for both functions. Not all languages keep their string zero terminated and keep track of the length. For C StringRef will call strlen so the cost is the same.
> > > > > 
> > > > > ```
> > > > > char *LLVMGetModuleIdentifier(LLVMModuleRef M, size_t *Length);
> > > > > void LLVMSetModuleIdentifier(LLVMModuleRef M, const char *ID, size_t Length);
> > > > > ```
> > > > No other function in llvm-c currently does this. I don't see how the nonexistent benefit of being able to add a non-zero-terminated string for a human-readable identifier overweighs the drawback of making the API inconsistent.
> > > Nitpicking: LLVMConstString* functions does include a Length argument, as does the new functions in D18727.
> > > 
> > > I have to agree it would be inconsistent, I will retract my comment and refer the issue to other stake holders in the C API.
> > Actually, several function in C API do that (for instance LLVMGetAsString and LLVMGetMDString), while other use zero terminated strings. The current state is rather inconsistent. If we are going to make it consistent, I'd go for the Length as out parameter.
> That's reasonable. Nicole, can you please change the API as suggested?
So, both zero terminated string and string with length as out param? Or just the latter?


http://reviews.llvm.org/D18736