Namespace code review

gen/src/namespace_organizer.rs

@@ -2,39 +2,127 @@ use crate::syntax::Api;
 use proc_macro2::Ident;
 use std::collections::BTreeMap;
 
-pub(crate) struct NamespaceEntries<'a> {
-    pub(crate) entries: Vec<&'a Api>,
-    pub(crate) children: BTreeMap<&'a Ident, NamespaceEntries<'a>>,
+pub struct NamespaceEntries<'a> {
+    entries: Vec<&'a Api>,
+    children: BTreeMap<&'a Ident, NamespaceEntries<'a>>,
 }
 
-pub(crate) fn sort_by_namespace(apis: &[Api]) -> NamespaceEntries {
-    let api_refs = apis.iter().collect::<Vec<_>>();
-    sort_by_inner_namespace(api_refs, 0)
-}
+impl<'a> NamespaceEntries<'a> {
+    pub fn new(apis: &'a [Api]) -> Self {
+        let api_refs = apis.iter().collect::<Vec<_>>();
+        Self::sort_by_inner_namespace(api_refs, 0)
+    }
+
+    pub fn entries(&self) -> &[&'a Api] {
+        &self.entries
+    }
 
-fn sort_by_inner_namespace(apis: Vec<&Api>, depth: usize) -> NamespaceEntries {
-    let mut root = NamespaceEntries {
-        entries: Vec::new(),
-        children: BTreeMap::new(),
-    };
-
-    let mut kids_by_child_ns = BTreeMap::new();
-    for api in apis {
-        if let Some(ns) = api.get_namespace() {
-            let first_ns_elem = ns.iter().nth(depth);
-            if let Some(first_ns_elem) = first_ns_elem {
-                let list = kids_by_child_ns.entry(first_ns_elem).or_insert(Vec::new());
-                list.push(api);
-                continue;
+    pub fn children(&self) -> impl Iterator<Item = (&&Ident, &NamespaceEntries)> {
+        self.children.iter()
+    }
+
+    fn sort_by_inner_namespace(apis: Vec<&'a Api>, depth: usize) -> Self {
+        let mut root = NamespaceEntries {
+            entries: Vec::new(),
+            children: BTreeMap::new(),
+        };
+
+        let mut kids_by_child_ns = BTreeMap::new();
+        for api in apis {
+            if let Some(ns) = api.get_namespace() {
+                let first_ns_elem = ns.iter().nth(depth);
+                if let Some(first_ns_elem) = first_ns_elem {
+                    let list = kids_by_child_ns.entry(first_ns_elem).or_insert(Vec::new());
+                    list.push(api);
+                    continue;
+                }
             }
+            root.entries.push(api);
+        }
+
+        for (k, v) in kids_by_child_ns.into_iter() {
+            root.children
+                .insert(k, Self::sort_by_inner_namespace(v, depth + 1));
         }
-        root.entries.push(api);
+
+        root
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::NamespaceEntries;
+    use crate::syntax::namespace::Namespace;
+    use crate::syntax::{Api, Doc, ExternType, Pair};
+    use proc_macro2::{Ident, Span};
+    use syn::Token;
+
+    #[test]
+    fn test_ns_entries_sort() {
+        let entries = vec![
+            make_api(None, "C"),
+            make_api(None, "A"),
+            make_api(Some("G"), "E"),
+            make_api(Some("D"), "F"),
+            make_api(Some("G"), "H"),
+            make_api(Some("D::K"), "L"),
+            make_api(Some("D::K"), "M"),
+            make_api(None, "B"),
+            make_api(Some("D"), "I"),
+            make_api(Some("D"), "J"),
+        ];
+        let ns = NamespaceEntries::new(&entries);
+        let root_entries = ns.entries();
+        assert_eq!(root_entries.len(), 3);
+        assert_ident(root_entries[0], "C");
+        assert_ident(root_entries[1], "A");
+        assert_ident(root_entries[2], "B");
+        let mut kids = ns.children();
+        let (d_id, d_nse) = kids.next().unwrap();
+        assert_eq!(d_id.to_string(), "D");
+        let (g_id, g_nse) = kids.next().unwrap();
+        assert_eq!(g_id.to_string(), "G");
+        assert!(kids.next().is_none());
+        let d_nse_entries = d_nse.entries();
+        assert_eq!(d_nse_entries.len(), 3);
+        assert_ident(d_nse_entries[0], "F");
+        assert_ident(d_nse_entries[1], "I");
+        assert_ident(d_nse_entries[2], "J");
+        let g_nse_entries = g_nse.entries();
+        assert_eq!(g_nse_entries.len(), 2);
+        assert_ident(g_nse_entries[0], "E");
+        assert_ident(g_nse_entries[1], "H");
+        let mut g_kids = g_nse.children();
+        assert!(g_kids.next().is_none());
+        let mut d_kids = d_nse.children();
+        let (k_id, k_nse) = d_kids.next().unwrap();
+        assert_eq!(k_id.to_string(), "K");
+        let k_nse_entries = k_nse.entries();
+        assert_eq!(k_nse_entries.len(), 2);
+        assert_ident(k_nse_entries[0], "L");
+        assert_ident(k_nse_entries[1], "M");
     }
 
-    for (k, v) in kids_by_child_ns.into_iter() {
-        root.children
-            .insert(k, sort_by_inner_namespace(v, depth + 1));
+    fn assert_ident(api: &Api, expected: &str) {
+        if let Api::CxxType(cxx_type) = api {
+            assert_eq!(cxx_type.ident.cxx.ident.to_string(), expected);
+        } else {
+            unreachable!()
+        }
     }
 
-    root
+    fn make_api(ns: Option<&str>, ident: &str) -> Api {
+        let ns = match ns {
+            Some(st) => Namespace::from_str(st),
+            None => Namespace::none(),
+        };
+        let ident = Pair::new(ns, Ident::new(ident, Span::call_site()));
+        Api::CxxType(ExternType {
+            doc: Doc::new(),
+            type_token: Token![type](Span::call_site()),
+            ident,
+            semi_token: Token![;](Span::call_site()),
+            trusted: true,
+        })
+    }
 }

gen/src/write.rs

@@ -1,4 +1,4 @@
-use crate::gen::namespace_organizer::{sort_by_namespace, NamespaceEntries};
+use crate::gen::namespace_organizer::NamespaceEntries;
 use crate::gen::out::OutFile;
 use crate::gen::{include, Opt};
 use crate::syntax::atom::Atom::{self, *};
@@ -30,7 +30,7 @@ pub(super) fn gen(apis: &[Api], types: &Types, opt: &Opt, header: bool) -> OutFi
 
     out.next_section();
 
-    let apis_by_namespace = sort_by_namespace(apis);
+    let apis_by_namespace = NamespaceEntries::new(apis);
 
     gen_namespace_contents(&apis_by_namespace, types, opt, header, out);
 
@@ -51,10 +51,10 @@ fn gen_namespace_contents(
     header: bool,
     out: &mut OutFile,
 ) {
-    let apis = &ns_entries.entries;
+    let apis = ns_entries.entries();
 
     out.next_section();
-    for api in apis {
+    for api in apis.iter() {
         match api {
             Api::Struct(strct) => write_struct_decl(out, &strct.ident.cxx.ident),
             Api::CxxType(ety) => write_struct_using(out, &ety.ident.cxx),
@@ -64,7 +64,7 @@ fn gen_namespace_contents(
     }
 
     let mut methods_for_type = HashMap::new();
-    for api in apis {
+    for api in apis.iter() {
         if let Api::RustFunction(efn) = api {
             if let Some(receiver) = &efn.sig.receiver {
                 methods_for_type
@@ -134,7 +134,7 @@ fn gen_namespace_contents(
 
     out.next_section();
 
-    for (child_ns, child_ns_entries) in &ns_entries.children {
+    for (child_ns, child_ns_entries) in ns_entries.children() {
         writeln!(out, "namespace {} {{", child_ns);
         gen_namespace_contents(&child_ns_entries, types, opt, header, out);
         writeln!(out, "}} // namespace {}", child_ns);

syntax/check.rs

@@ -211,9 +211,7 @@ fn check_api_type(cx: &mut Check, ety: &ExternType) {
 
     if let Some(reason) = cx.types.required_trivial.get(&ety.ident.rust) {
         let what = match reason {
-            TrivialReason::StructField(strct) => {
-                format!("a field of `{}`", strct.ident.cxx.to_fully_qualified())
-            }
+            TrivialReason::StructField(strct) => format!("a field of `{}`", strct.ident.rust),
             TrivialReason::FunctionArgument(efn) => format!("an argument of `{}`", efn.ident.rust),
             TrivialReason::FunctionReturn(efn) => format!("a return value of `{}`", efn.ident.rust),
         };

syntax/impls.rs

@@ -300,25 +300,29 @@ impl Borrow<Type> for &Impl {
 
 impl Pair {
     /// Use this constructor when the item can't have a different
-    /// name in Rust and C++. For cases where #[rust_name] and similar
-    /// attributes can be used, construct the object by hand.
+    /// name in Rust and C++.
     pub fn new(ns: Namespace, ident: Ident) -> Self {
         Self {
             rust: ident.clone(),
             cxx: CppName::new(ns, ident),
         }
     }
+
+    /// Use this constructor when attributes such as #[rust_name]
+    /// can be used to potentially give a different name in Rust vs C++.
+    pub fn new_from_differing_names(ns: Namespace, cxx_ident: Ident, rust_ident: Ident) -> Self {
+        Self {
+            rust: rust_ident,
+            cxx: CppName::new(ns, cxx_ident),
+        }
+    }
 }
 
 impl ResolvableName {
     pub fn new(ident: Ident) -> Self {
         Self { rust: ident }
     }
 
-    pub fn from_pair(pair: Pair) -> Self {
-        Self { rust: pair.rust }
-    }
-
     pub fn make_self(span: Span) -> Self {
         Self {
             rust: Token![Self](span).into(),
@@ -357,9 +361,7 @@ impl CppName {
         Self { ns, ident }
     }
 
-    fn iter_all_segments(
-        &self,
-    ) -> std::iter::Chain<std::slice::Iter<Ident>, std::iter::Once<&Ident>> {
+    fn iter_all_segments(&self) -> impl Iterator<Item = &Ident> {
         self.ns.iter().chain(std::iter::once(&self.ident))
     }
 

syntax/namespace.rs

@@ -1,4 +1,6 @@
 use crate::syntax::qualified::QualifiedName;
+#[cfg(test)]
+use proc_macro2::Span;
 use quote::IdentFragment;
 use std::fmt::{self, Display};
 use std::slice::Iter;
@@ -36,6 +38,16 @@ impl Namespace {
         input.parse::<Option<Token![,]>>()?;
         Ok(ns)
     }
+
+    #[cfg(test)]
+    pub fn from_str(ns: &str) -> Self {
+        Namespace {
+            segments: ns
+                .split("::")
+                .map(|x| Ident::new(x, Span::call_site()))
+                .collect(),
+        }
+    }
 }
 
 impl Parse for Namespace {

syntax/parse.rs

@@ -3,7 +3,7 @@ use crate::syntax::file::{Item, ItemForeignMod};
 use crate::syntax::report::Errors;
 use crate::syntax::Atom::*;
 use crate::syntax::{
-    attrs, error, Api, CppName, Doc, Enum, ExternFn, ExternType, Impl, Include, IncludeKind, Lang,
+    attrs, error, Api, Doc, Enum, ExternFn, ExternType, Impl, Include, IncludeKind, Lang,
     Namespace, Pair, Receiver, Ref, ResolvableName, Signature, Slice, Struct, Ty1, Type, TypeAlias,
     Var, Variant,
 };
@@ -245,7 +245,7 @@ fn parse_foreign_mod(
             if let Api::CxxFunction(efn) | Api::RustFunction(efn) = item {
                 if let Some(receiver) = &mut efn.receiver {
                     if receiver.ty.is_self() {
-                        receiver.ty = ResolvableName::from_pair(single_type.clone());
+                        receiver.ty = ResolvableName::new(single_type.rust.clone());
                     }
                 }
             }
@@ -398,10 +398,11 @@ fn parse_extern_fn(
     let throws = throws_tokens.is_some();
     let unsafety = foreign_fn.sig.unsafety;
     let fn_token = foreign_fn.sig.fn_token;
-    let ident = Pair {
-        cxx: CppName::new(ns, cxx_name.unwrap_or(foreign_fn.sig.ident.clone())),
-        rust: rust_name.unwrap_or(foreign_fn.sig.ident.clone()),
-    };
+    let ident = Pair::new_from_differing_names(
+        ns,
+        cxx_name.unwrap_or(foreign_fn.sig.ident.clone()),
+        rust_name.unwrap_or(foreign_fn.sig.ident.clone()),
+    );
     let paren_token = foreign_fn.sig.paren_token;
     let semi_token = foreign_fn.semi_token;
     let api_function = match lang {

tests/BUCK

@@ -15,7 +15,6 @@ rust_library(
         "ffi/extra.rs",
         "ffi/lib.rs",
         "ffi/module.rs",
-        "ffi/class_in_ns.rs",
     ],
     crate = "cxx_test_suite",
     deps = [
@@ -31,7 +30,6 @@ cxx_library(
         ":bridge/source",
         ":extra/source",
         ":module/source",
-        ":class_in_ns/source",
     ],
     headers = {
         "ffi/lib.rs.h": ":bridge/header",
@@ -54,8 +52,3 @@ rust_cxx_bridge(
     name = "module",
     src = "ffi/module.rs",
 )
-
-rust_cxx_bridge(
-    name = "class_in_ns",
-    src = "ffi/class_in_ns.rs",
-)

tests/BUILD

@@ -18,7 +18,6 @@ rust_library(
         "ffi/extra.rs",
         "ffi/lib.rs",
         "ffi/module.rs",
-        "ffi/class_in_ns.rs",
     ],
     deps = [
         ":impl",
@@ -33,7 +32,6 @@ cc_library(
         ":bridge/source",
         ":extra/source",
         ":module/source",
-        ":class_in_ns/source",
     ],
     hdrs = ["ffi/tests.h"],
     deps = [
@@ -59,9 +57,3 @@ rust_cxx_bridge(
     src = "ffi/module.rs",
     deps = [":impl"],
 )
-
-rust_cxx_bridge(
-    name = "class_in_ns",
-    src = "ffi/class_in_ns.rs",
-    deps = [":impl"],
-)

tests/ffi/build.rs

@@ -6,7 +6,7 @@ fn main() {
     }
 
     CFG.include_prefix = "tests/ffi";
-    let sources = vec!["lib.rs", "extra.rs", "module.rs", "class_in_ns.rs"];
+    let sources = vec!["lib.rs", "extra.rs", "module.rs"];
     cxx_build::bridges(sources)
         .file("tests.cc")
         .flag_if_supported(cxxbridge_flags::STD)