Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overloaded function renaming might cause conflicts #1316

Open
hfiguiere opened this issue Aug 4, 2023 · 3 comments
Open

Overloaded function renaming might cause conflicts #1316

hfiguiere opened this issue Aug 4, 2023 · 3 comments
Labels
bug Something isn't working good first issue Good for newcomers has-test-case Test case written and checked in

Comments

@hfiguiere
Copy link

hfiguiere commented Aug 4, 2023

Describe the bug

Overloaded function renaming might cause conflicts

To Reproduce

See #1317

I have a class:

class Image {
public:
  static uint64_t byteSwap(uint64_t value, bool bSwap);
  static uint32_t byteSwap(uint32_t value, bool bSwap);
  static uint16_t byteSwap(uint16_t value, bool bSwap);
  static uint16_t byteSwap2(const DataBuf& buf, size_t offset, bool bSwap);
};

calling generate! on it will cause byteSwap2 to be generated twice. One for the overload, one for the actual byteSwap2.

pub unsafe fn byteSwap(value: u64, bSwap: bool) -> u64 { }
pub unsafe fn byteSwap1(value: u32, bSwap: bool) -> u32 { }
pub unsafe fn byteSwap2(value: u16, bSwap: bool) -> u16 { }
pub unsafe fn byteSwap2(buf: &root::Exiv2::DataBuf,
       offset: usize,
       bSwap: bool,
) -> u16 {}

This is because the renaming of the return u16 overload is byteSwap2 and there is an actual byteSwap2 function.

Expected behavior

Find a better renaming.

Or throw an error from the macro

Additional context

hfiguiere added a commit to hfiguiere/autocxx that referenced this issue Aug 4, 2023
@hfiguiere hfiguiere changed the title overloaded functon renaming might cause conflicts Overloaded functon renaming might cause conflicts Aug 4, 2023
@hfiguiere hfiguiere changed the title Overloaded functon renaming might cause conflicts Overloaded function renaming might cause conflicts Aug 4, 2023
@adetaylor
Copy link
Collaborator

Yeah, this isn't great and as the manual says, this all needs to be overhauled a bit. Meanwhile though I do certainly consider this a bug, and I'd accept a pull request for a temporary solution, e.g. if SomeOverloadedFunction2 already exists, then use a different suffix such as SomeOverloadedFunction_autocxx_overload2. I'm unlikely to get a chance to do this any time soon, but this might be a fairly easy fix so I'm going to mark it as Good First Issue in case someone else can help out.

@adetaylor adetaylor added bug Something isn't working good first issue Good for newcomers has-test-case Test case written and checked in labels Sep 1, 2023
@adetaylor
Copy link
Collaborator

Closely related to #995

adetaylor added a commit that referenced this issue Sep 12, 2023
Work towards #1316 and #995.

For example,
  thingy_autocxx_overload1
This doesn't currently work because ApiVec rejects duplicated APIs at
an earlier stage based upon bindgen function names.
@minghuaw
Copy link

minghuaw commented Dec 22, 2023

Just sharing some random thoughts on overloaded functions. Would the following options be better?

  1. Generate macros for the overloaded functions?

    A potential problem I can see is overloads that have the same number of args but the args are of different types, like the three byteSwap functions mentioned above.

  2. Generate trait for the overloaded functions?

    What I am picturing is something like below. I guess a potential problem would be generating multiple traits of the same name.

struct Image;

pub trait ByteSwap<Args> {
    type Output;

    fn byte_swap(args: Args) -> Self::Output;
}

impl ByteSwap<(u64, bool)> for Image {
    type Output = u64;

    fn byte_swap(args: (u64, bool)) -> u64 { todo!() }
}


impl ByteSwap<(u32, bool)> for Image {
    type Output = u32;

    fn byte_swap(args: (u32, bool)) -> u32 { todo!() }
}

impl ByteSwap<(u16, bool)> for Image {
    type Output = u16;

    fn byte_swap(args: (u16, bool)) -> u16 { todo!() }
}

Or generate a trait for the args so that the trait doesn't need to be public

mod ffi {
    pub struct Image;
    
    impl Image {
        fn byte_swap<Args: ByteSwapArgs<Image>>(args: Args) -> Args::Output {
            ByteSwapArgs::<Image>::perform_byte_swap(args)
        }
    }
    
    trait ByteSwapArgs<T> {
        type Output;
    
        fn perform_byte_swap(self) -> Self::Output;
    }
    
    impl ByteSwapArgs<Image> for (u64, bool) {
        type Output = u64;
        
        fn perform_byte_swap(self) -> u64 { todo!() }
    }
    
    impl ByteSwapArgs<Image> for (u32, bool) {
        type Output = u32;
        
        fn perform_byte_swap(self) -> u32 { todo!() }
    }
    
    impl ByteSwapArgs<Image> for (u16, bool) {
        type Output = u16;
        
        fn perform_byte_swap(self) -> u16 { todo!() }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers has-test-case Test case written and checked in
Projects
None yet
Development

No branches or pull requests

3 participants