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

[Eric Christopher via llvm-commits]

On Mon, Apr 19, 2021 at 9:49 AM Jeroen Dobbelaere via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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`, ...).
>

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.

Thoughts?

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210427/223d2f13/attachment.html>