[PATCH] D99173: Intrinsic::getName: require a Module argument

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added inline comments.


================
Comment at: llvm/include/llvm-c/Core.h:2545
+ */
+const char *LLVMIntrinsicCopyOverloadedName2(unsigned ID,
+                                             LLVMTypeRef *ParamTypes,
----------------
fhahn wrote:
> Not sure about the name, but I think Eric's suggestion of `LLVMIntrinsicCopyOverloadedNameWithModule` on the list seems a bit more descriptive to me. Not sure what others think and naming is hard.
There are already 17 other deprecations in favor of a '...2' name. (ex. `LLVMBuildLoad2`, `LLVMMDStringInContext2`, ...)
Which seems to be far more than the deprecations in favor of a different name. (ex. `LLVMGetUnnamedAddress` instead of `LLVMHasUnnamedAddr2`)

For this particular case, my impression is that the '...2' name is easier to grasp and to understand what it will do. In the  '...WithModule', somehow I get the feeling the the module will also be copied ?

Maybe it is better to completely deprecate the original version ?

Also I am thinking of moving the ModuleRef to the beginning. What do you think ?




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99173/new/

https://reviews.llvm.org/D99173