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

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: llvm/include/llvm-c/Core.h:2545
+ */
+const char *LLVMIntrinsicCopyOverloadedName2(unsigned ID,
+                                             LLVMTypeRef *ParamTypes,
----------------
jeroen.dobbelaere wrote:
> 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 ?
> 
> 
I mostly think that the "2" names aren't particularly enlightening or helpful. I'd prefer something that doesn't give us a "3" when we need or want a new argument. WithModule is perhaps not a good naming, as Florian said it's hard. Any other thoughts? ID or ModuleRef should be first. Might want to take a look at some of the other C++ intrinsic calls to get a feel for it. 


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

https://reviews.llvm.org/D99173