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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a comment.

Thanks for the feedback.



================
Comment at: llvm/include/llvm-c/Core.h:2545
+ */
+const char *LLVMIntrinsicCopyOverloadedName2(unsigned ID,
+                                             LLVMTypeRef *ParamTypes,
----------------
echristo wrote:
> 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. 
> 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. 

I already moved the ModuleRef to the front, similar as in `LLVMGetIntrinsicDeclaration(Mod, ID, ..)`.
I also looked at the history of `Core.h` to get a feeling about the naming. My impression is that api updates result in a new different name when that makes the api cleaner as well. In other cases, the need to add arguments resulted in adding a `2` as suffix, and deprecating the original one. (See `LLVMBuildGEP2`, `LLVMBuildStructGEP2`, `LLVMGetValueName2`, ...).



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

https://reviews.llvm.org/D99173