<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 19, 2021 at 9:49 AM Jeroen Dobbelaere via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">jeroen.dobbelaere added a comment.<br>
<br>
Thanks for the feedback.<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm-c/Core.h:2545<br>
+ */<br>
+const char *LLVMIntrinsicCopyOverloadedName2(unsigned ID,<br>
+                                             LLVMTypeRef *ParamTypes,<br>
----------------<br>
echristo wrote:<br>
> jeroen.dobbelaere wrote:<br>
> > fhahn wrote:<br>
> > > 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.<br>
> > There are already 17 other deprecations in favor of a '...2' name. (ex. `LLVMBuildLoad2`, `LLVMMDStringInContext2`, ...)<br>
> > Which seems to be far more than the deprecations in favor of a different name. (ex. `LLVMGetUnnamedAddress` instead of `LLVMHasUnnamedAddr2`)<br>
> > <br>
> > 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 ?<br>
> > <br>
> > Maybe it is better to completely deprecate the original version ?<br>
> > <br>
> > Also I am thinking of moving the ModuleRef to the beginning. What do you think ?<br>
> > <br>
> > <br>
> 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. <br>
> 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. <br>
<br>
I already moved the ModuleRef to the front, similar as in `LLVMGetIntrinsicDeclaration(Mod, ID, ..)`.<br>
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`, ...).<br></blockquote><div><br></div><div>I know, I was responsible for a lot of that policy :) I just didn't particularly like it at the time, but didn't have any better ideas so I was hoping this might be a good chance to change that.</div><div><br></div><div>Thoughts?</div><div><br></div><div>-eric </div></div></div>