Attempt to better name overloads

engine/src/conversion/analysis/fun/mod.rs

@@ -309,7 +309,7 @@ impl<'a> FnAnalyzer<'a> {
             type_converter: TypeConverter::new(config, &apis),
             bridge_name_tracker: BridgeNameTracker::new(),
             config,
-            overload_trackers_by_mod: HashMap::new(),
+            overload_trackers_by_mod: Self::build_overload_trackers(&apis),
             pod_safe_types: Self::build_pod_safe_type_set(&apis),
             moveit_safe_types: Self::build_correctly_sized_type_set(&apis),
             subclasses_by_superclass: subclass::subclasses_by_superclass(&apis),
@@ -365,6 +365,20 @@ impl<'a> FnAnalyzer<'a> {
             .collect()
     }
 
+    /// For calculation of subsequent overloads, we want to work out the
+    /// names that we ideally would use. e.g. if we find func(int)
+    /// and func(float) we would normally call them func and func1,
+    /// but if there's already a func1 in the data structure returned
+    /// by this function then we'll call it something else.
+    fn build_overload_trackers(apis: &ApiVec<PodPhase>) -> HashMap<Namespace, OverloadTracker> {
+        let all_namespaces: HashSet<&Namespace> =
+            apis.iter().map(|api| api.name().get_namespace()).collect();
+        all_namespaces
+            .into_iter()
+            .map(|ns| (ns.clone(), OverloadTracker::new(apis, ns)))
+            .collect()
+    }
+
     /// Return the set of 'moveit safe' types. That must include only types where
     /// the size is known to be correct.
     fn build_correctly_sized_type_set(apis: &ApiVec<PodPhase>) -> HashSet<QualifiedName> {
@@ -704,6 +718,36 @@ impl<'a> FnAnalyzer<'a> {
         name.name
     }
 
+    /// Work out the ideal Rust name for a given function, taking account
+    /// of overloads and Rust idents.
+    pub(crate) fn ideal_rust_name(name: &ApiName, fun: &FuncToConvert) -> String {
+        let cpp_name = name.cpp_name_if_present().cloned();
+        // bindgen may have mangled the name either because it's invalid Rust
+        // syntax (e.g. a keyword like 'async') or it's an overload.
+        // If the former, we respect that mangling. If the latter, we don't,
+        // because we'll add our own overload counting mangling later.
+        // Cases:
+        //   function, IRN=foo,    CN=<none>                    output: foo    case 1
+        //   function, IRN=move_,  CN=move   (keyword problem)  output: move_  case 2
+        //   function, IRN=foo1,   CN=foo    (overload)         output: foo    case 3
+        //   method,   IRN=A_foo,  CN=foo                       output: foo    case 4
+        //   method,   IRN=A_move, CN=move   (keyword problem)  output: move_  case 5
+        //   method,   IRN=A_foo1, CN=foo    (overload)         output: foo    case 6
+        let initial_rust_name = fun.ident.to_string();
+        match &cpp_name {
+            None => initial_rust_name, // case 1
+            Some(cpp_name) => {
+                if initial_rust_name.ends_with('_') {
+                    initial_rust_name // case 2
+                } else if validate_ident_ok_for_rust(cpp_name).is_err() {
+                    format!("{cpp_name}_") // case 5
+                } else {
+                    cpp_name.to_string() // cases 3, 4, 6
+                }
+            }
+        }
+    }
+
     /// Determine how to materialize a function.
     ///
     /// The main job here is to determine whether a function can simply be noted
@@ -775,29 +819,9 @@ impl<'a> FnAnalyzer<'a> {
 
         // End of parameter processing.
         // Work out naming, part one.
-        // bindgen may have mangled the name either because it's invalid Rust
-        // syntax (e.g. a keyword like 'async') or it's an overload.
-        // If the former, we respect that mangling. If the latter, we don't,
-        // because we'll add our own overload counting mangling later.
-        // Cases:
-        //   function, IRN=foo,    CN=<none>                    output: foo    case 1
-        //   function, IRN=move_,  CN=move   (keyword problem)  output: move_  case 2
-        //   function, IRN=foo1,   CN=foo    (overload)         output: foo    case 3
-        //   method,   IRN=A_foo,  CN=foo                       output: foo    case 4
-        //   method,   IRN=A_move, CN=move   (keyword problem)  output: move_  case 5
-        //   method,   IRN=A_foo1, CN=foo    (overload)         output: foo    case 6
-        let ideal_rust_name = match &cpp_name {
-            None => initial_rust_name, // case 1
-            Some(cpp_name) => {
-                if initial_rust_name.ends_with('_') {
-                    initial_rust_name // case 2
-                } else if validate_ident_ok_for_rust(cpp_name).is_err() {
-                    format!("{cpp_name}_") // case 5
-                } else {
-                    cpp_name.to_string() // cases 3, 4, 6
-                }
-            }
-        };
+        // See what we'd ideally call this in Rust, which we might
+        // later need to alter based on overloads.
+        let ideal_rust_name = Self::ideal_rust_name(&name, fun);
 
         // Let's spend some time figuring out the kind of this function (i.e. method,
         // virtual function, etc.)
@@ -1510,7 +1534,7 @@ impl<'a> FnAnalyzer<'a> {
     }
 
     fn get_overload_name(&mut self, ns: &Namespace, type_ident: &str, rust_name: String) -> String {
-        let overload_tracker = self.overload_trackers_by_mod.entry(ns.clone()).or_default();
+        let overload_tracker = self.overload_trackers_by_mod.get_mut(ns).unwrap();
         overload_tracker.get_method_real_name(type_ident, rust_name)
     }
 
@@ -1624,7 +1648,7 @@ impl<'a> FnAnalyzer<'a> {
     }
 
     fn get_function_overload_name(&mut self, ns: &Namespace, ideal_rust_name: String) -> String {
-        let overload_tracker = self.overload_trackers_by_mod.entry(ns.clone()).or_default();
+        let overload_tracker = self.overload_trackers_by_mod.get_mut(ns).unwrap();
         overload_tracker.get_function_real_name(ideal_rust_name)
     }
 

engine/src/conversion/analysis/fun/overload_tracker.rs

@@ -6,7 +6,15 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use std::collections::HashMap;
+use indexmap::IndexMap as HashMap;
+use indexmap::IndexSet as HashSet;
+
+use crate::conversion::analysis::pod::PodPhase;
+use crate::conversion::api::Api;
+use crate::conversion::apivec::ApiVec;
+use crate::types::Namespace;
+
+use super::FnAnalyzer;
 
 type Offsets = HashMap<String, usize>;
 
@@ -20,13 +28,40 @@ type Offsets = HashMap<String, usize>;
 /// If bindgen adds a suffix it will be included in 'found_name'
 /// but not 'original_name' which is an annotation added by our autocxx-bindgen
 /// fork.
-#[derive(Default)]
 pub(crate) struct OverloadTracker {
+    names_to_avoid: HashSet<String>,
+    names_to_avoid_by_type: HashMap<String, HashSet<String>>,
     offset_by_name: Offsets,
     offset_by_type_and_name: HashMap<String, Offsets>,
 }
 
 impl OverloadTracker {
+    pub(crate) fn new(apis: &ApiVec<PodPhase>, namespace: &Namespace) -> Self {
+        let mut names_to_avoid = HashSet::new();
+        let mut names_to_avoid_by_type: HashMap<String, HashSet<String>> = HashMap::new();
+        for api in apis
+            .iter()
+            .filter(|api| api.name().get_namespace() == namespace)
+        {
+            if let Api::Function { name, fun, .. } = api {
+                let ideal = FnAnalyzer::ideal_rust_name(name, fun);
+                let set_to_change = match fun.self_ty {
+                    None => &mut names_to_avoid,
+                    Some(ref self_ty) => names_to_avoid_by_type
+                        .entry(self_ty.get_final_item().to_string())
+                        .or_default(),
+                };
+                set_to_change.insert(ideal);
+            }
+        }
+        Self {
+            names_to_avoid,
+            names_to_avoid_by_type,
+            offset_by_name: Default::default(),
+            offset_by_type_and_name: Default::default(),
+        }
+    }
+
     pub(crate) fn get_function_real_name(&mut self, found_name: String) -> String {
         self.get_name(None, found_name)
     }
@@ -36,39 +71,57 @@ impl OverloadTracker {
     }
 
     fn get_name(&mut self, type_name: Option<&str>, cpp_method_name: String) -> String {
-        let registry = match type_name {
-            Some(type_name) => self
-                .offset_by_type_and_name
-                .entry(type_name.to_string())
-                .or_default(),
-            None => &mut self.offset_by_name,
+        let empty_set = HashSet::new();
+        let (registry, to_avoid) = match type_name {
+            Some(type_name) => (
+                self.offset_by_type_and_name
+                    .entry(type_name.to_string())
+                    .or_default(),
+                self.names_to_avoid_by_type
+                    .get(&type_name.to_string())
+                    .unwrap_or(&empty_set),
+            ),
+            None => (&mut self.offset_by_name, &self.names_to_avoid),
         };
+        log::info!(
+            "Considering {:?}, {}, to avoid contains {:?}",
+            type_name,
+            cpp_method_name,
+            to_avoid
+        );
         let offset = registry.entry(cpp_method_name.clone()).or_default();
         let this_offset = *offset;
         *offset += 1;
         if this_offset == 0 {
             cpp_method_name
         } else {
-            format!("{cpp_method_name}{this_offset}")
+            let provisional = format!("{cpp_method_name}{this_offset}");
+            if to_avoid.contains(&provisional) {
+                format!("{cpp_method_name}_autocxx_overload{this_offset}")
+            } else {
+                provisional
+            }
         }
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use crate::{conversion::apivec::ApiVec, types::Namespace};
+
     use super::OverloadTracker;
 
     #[test]
     fn test_by_function() {
-        let mut ot = OverloadTracker::default();
-        assert_eq!(ot.get_function_real_name("bob".into()), "bob");
+        let mut ot = OverloadTracker::new(&ApiVec::new(), &Namespace::new());
+        assert_eq!(ot.get_function_real_name("bob".into()), "bob",);
         assert_eq!(ot.get_function_real_name("bob".into()), "bob1");
         assert_eq!(ot.get_function_real_name("bob".into()), "bob2");
     }
 
     #[test]
     fn test_by_method() {
-        let mut ot = OverloadTracker::default();
+        let mut ot = OverloadTracker::new(&ApiVec::new(), &Namespace::new());
         assert_eq!(ot.get_method_real_name("Ty1", "bob".into()), "bob");
         assert_eq!(ot.get_method_real_name("Ty1", "bob".into()), "bob1");
         assert_eq!(ot.get_method_real_name("Ty2", "bob".into()), "bob");

integration-tests/tests/integration_test.rs

@@ -2280,8 +2280,28 @@ fn test_overload_functions() {
 }
 
 #[test]
-#[ignore] // At present, bindgen generates two separate 'daft1'
-          // functions here, and there's not much we can do about that.
+fn test_overload_single_numeric_function() {
+    let cxx = indoc! {"
+        void daft1(uint32_t) {}
+        void daft(float) {}
+        void daft(uint32_t) {}
+    "};
+    let hdr = indoc! {"
+        #include <cstdint>
+        void daft1(uint32_t a);
+        void daft(uint32_t a);
+        void daft(float a);
+    "};
+    let rs = quote! {
+        use ffi::ToCppString;
+        ffi::daft(32);
+        ffi::daft1(8);
+        ffi::daft_autocxx_overload1(3.2);
+    };
+    run_test(cxx, hdr, rs, &["daft", "daft1"], &[]);
+}
+
+#[test]
 fn test_overload_numeric_functions() {
     // Because bindgen deals with conflicting overloaded functions by
     // appending a numeric suffix, let's see if we can cope.