Skip to content

Commit 6791c39

Browse files
dtolnaydtolnay
authored
Merge pull request #382 from adetaylor/namespace-code-review
Namespace code review
2 parents 71b34be + 451ec9f commit 6791c39

File tree

14 files changed

+162
-91
lines changed

14 files changed

+162
-91
lines changed

‎gen/src/namespace_organizer.rs

Lines changed: 114 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,39 +2,127 @@ use crate::syntax::Api;
22
use proc_macro2::Ident;
33
use std::collections::BTreeMap;
44

5-
pub(crate) struct NamespaceEntries<'a> {
6-
pub(crate) entries: Vec<&'a Api>,
7-
pub(crate) children: BTreeMap<&'a Ident, NamespaceEntries<'a>>,
5+
pub struct NamespaceEntries<'a> {
6+
entries: Vec<&'a Api>,
7+
children: BTreeMap<&'a Ident, NamespaceEntries<'a>>,
88
}
99

10-
pub(crate) fn sort_by_namespace(apis: &[Api]) -> NamespaceEntries {
11-
let api_refs = apis.iter().collect::<Vec<_>>();
12-
sort_by_inner_namespace(api_refs, 0)
13-
}
10+
impl<'a> NamespaceEntries<'a> {
11+
pub fn new(apis: &'a [Api]) -> Self {
12+
let api_refs = apis.iter().collect::<Vec<_>>();
13+
Self::sort_by_inner_namespace(api_refs, 0)
14+
}
15+
16+
pub fn entries(&self) -> &[&'a Api] {
17+
&self.entries
18+
}
1419

15-
fn sort_by_inner_namespace(apis: Vec<&Api>, depth: usize) -> NamespaceEntries {
16-
let mut root = NamespaceEntries {
17-
entries: Vec::new(),
18-
children: BTreeMap::new(),
19-
};
20-
21-
let mut kids_by_child_ns = BTreeMap::new();
22-
for api in apis {
23-
if let Some(ns) = api.get_namespace() {
24-
let first_ns_elem = ns.iter().nth(depth);
25-
if let Some(first_ns_elem) = first_ns_elem {
26-
let list = kids_by_child_ns.entry(first_ns_elem).or_insert(Vec::new());
27-
list.push(api);
28-
continue;
20+
pub fn children(&self) -> impl Iterator<Item = (&&Ident, &NamespaceEntries)> {
21+
self.children.iter()
22+
}
23+
24+
fn sort_by_inner_namespace(apis: Vec<&'a Api>, depth: usize) -> Self {
25+
let mut root = NamespaceEntries {
26+
entries: Vec::new(),
27+
children: BTreeMap::new(),
28+
};
29+
30+
let mut kids_by_child_ns = BTreeMap::new();
31+
for api in apis {
32+
if let Some(ns) = api.get_namespace() {
33+
let first_ns_elem = ns.iter().nth(depth);
34+
if let Some(first_ns_elem) = first_ns_elem {
35+
let list = kids_by_child_ns.entry(first_ns_elem).or_insert(Vec::new());
36+
list.push(api);
37+
continue;
38+
}
2939
}
40+
root.entries.push(api);
41+
}
42+
43+
for (k, v) in kids_by_child_ns.into_iter() {
44+
root.children
45+
.insert(k, Self::sort_by_inner_namespace(v, depth + 1));
3046
}
31-
root.entries.push(api);
47+
48+
root
49+
}
50+
}
51+
52+
#[cfg(test)]
53+
mod tests {
54+
use super::NamespaceEntries;
55+
use crate::syntax::namespace::Namespace;
56+
use crate::syntax::{Api, Doc, ExternType, Pair};
57+
use proc_macro2::{Ident, Span};
58+
use syn::Token;
59+
60+
#[test]
61+
fn test_ns_entries_sort() {
62+
let entries = vec![
63+
make_api(None, "C"),
64+
make_api(None, "A"),
65+
make_api(Some("G"), "E"),
66+
make_api(Some("D"), "F"),
67+
make_api(Some("G"), "H"),
68+
make_api(Some("D::K"), "L"),
69+
make_api(Some("D::K"), "M"),
70+
make_api(None, "B"),
71+
make_api(Some("D"), "I"),
72+
make_api(Some("D"), "J"),
73+
];
74+
let ns = NamespaceEntries::new(&entries);
75+
let root_entries = ns.entries();
76+
assert_eq!(root_entries.len(), 3);
77+
assert_ident(root_entries[0], "C");
78+
assert_ident(root_entries[1], "A");
79+
assert_ident(root_entries[2], "B");
80+
let mut kids = ns.children();
81+
let (d_id, d_nse) = kids.next().unwrap();
82+
assert_eq!(d_id.to_string(), "D");
83+
let (g_id, g_nse) = kids.next().unwrap();
84+
assert_eq!(g_id.to_string(), "G");
85+
assert!(kids.next().is_none());
86+
let d_nse_entries = d_nse.entries();
87+
assert_eq!(d_nse_entries.len(), 3);
88+
assert_ident(d_nse_entries[0], "F");
89+
assert_ident(d_nse_entries[1], "I");
90+
assert_ident(d_nse_entries[2], "J");
91+
let g_nse_entries = g_nse.entries();
92+
assert_eq!(g_nse_entries.len(), 2);
93+
assert_ident(g_nse_entries[0], "E");
94+
assert_ident(g_nse_entries[1], "H");
95+
let mut g_kids = g_nse.children();
96+
assert!(g_kids.next().is_none());
97+
let mut d_kids = d_nse.children();
98+
let (k_id, k_nse) = d_kids.next().unwrap();
99+
assert_eq!(k_id.to_string(), "K");
100+
let k_nse_entries = k_nse.entries();
101+
assert_eq!(k_nse_entries.len(), 2);
102+
assert_ident(k_nse_entries[0], "L");
103+
assert_ident(k_nse_entries[1], "M");
32104
}
33105

34-
for (k, v) in kids_by_child_ns.into_iter() {
35-
root.children
36-
.insert(k, sort_by_inner_namespace(v, depth + 1));
106+
fn assert_ident(api: &Api, expected: &str) {
107+
if let Api::CxxType(cxx_type) = api {
108+
assert_eq!(cxx_type.ident.cxx.ident.to_string(), expected);
109+
} else {
110+
unreachable!()
111+
}
37112
}
38113

39-
root
114+
fn make_api(ns: Option<&str>, ident: &str) -> Api {
115+
let ns = match ns {
116+
Some(st) => Namespace::from_str(st),
117+
None => Namespace::none(),
118+
};
119+
let ident = Pair::new(ns, Ident::new(ident, Span::call_site()));
120+
Api::CxxType(ExternType {
121+
doc: Doc::new(),
122+
type_token: Token![type](Span::call_site()),
123+
ident,
124+
semi_token: Token![;](Span::call_site()),
125+
trusted: true,
126+
})
127+
}
40128
}

‎gen/src/write.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::gen::namespace_organizer::{sort_by_namespace, NamespaceEntries};
1+
use crate::gen::namespace_organizer::NamespaceEntries;
22
use crate::gen::out::OutFile;
33
use crate::gen::{include, Opt};
44
use crate::syntax::atom::Atom::{self, *};
@@ -30,7 +30,7 @@ pub(super) fn gen(apis: &[Api], types: &Types, opt: &Opt, header: bool) -> OutFi
3030

3131
out.next_section();
3232

33-
let apis_by_namespace = sort_by_namespace(apis);
33+
let apis_by_namespace = NamespaceEntries::new(apis);
3434

3535
gen_namespace_contents(&apis_by_namespace, types, opt, header, out);
3636

@@ -51,10 +51,10 @@ fn gen_namespace_contents(
5151
header: bool,
5252
out: &mut OutFile,
5353
) {
54-
let apis = &ns_entries.entries;
54+
let apis = ns_entries.entries();
5555

5656
out.next_section();
57-
for api in apis {
57+
for api in apis.iter() {
5858
match api {
5959
Api::Struct(strct) => write_struct_decl(out, &strct.ident.cxx.ident),
6060
Api::CxxType(ety) => write_struct_using(out, &ety.ident.cxx),
@@ -64,7 +64,7 @@ fn gen_namespace_contents(
6464
}
6565

6666
let mut methods_for_type = HashMap::new();
67-
for api in apis {
67+
for api in apis.iter() {
6868
if let Api::RustFunction(efn) = api {
6969
if let Some(receiver) = &efn.sig.receiver {
7070
methods_for_type
@@ -134,7 +134,7 @@ fn gen_namespace_contents(
134134

135135
out.next_section();
136136

137-
for (child_ns, child_ns_entries) in &ns_entries.children {
137+
for (child_ns, child_ns_entries) in ns_entries.children() {
138138
writeln!(out, "namespace {} {{", child_ns);
139139
gen_namespace_contents(&child_ns_entries, types, opt, header, out);
140140
writeln!(out, "}} // namespace {}", child_ns);

‎syntax/check.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,7 @@ fn check_api_type(cx: &mut Check, ety: &ExternType) {
211211

212212
if let Some(reason) = cx.types.required_trivial.get(&ety.ident.rust) {
213213
let what = match reason {
214-
TrivialReason::StructField(strct) => {
215-
format!("a field of `{}`", strct.ident.cxx.to_fully_qualified())
216-
}
214+
TrivialReason::StructField(strct) => format!("a field of `{}`", strct.ident.rust),
217215
TrivialReason::FunctionArgument(efn) => format!("an argument of `{}`", efn.ident.rust),
218216
TrivialReason::FunctionReturn(efn) => format!("a return value of `{}`", efn.ident.rust),
219217
};

‎syntax/impls.rs

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -300,25 +300,29 @@ impl Borrow<Type> for &Impl {
300300

301301
impl Pair {
302302
/// Use this constructor when the item can't have a different
303-
/// name in Rust and C++. For cases where #[rust_name] and similar
304-
/// attributes can be used, construct the object by hand.
303+
/// name in Rust and C++.
305304
pub fn new(ns: Namespace, ident: Ident) -> Self {
306305
Self {
307306
rust: ident.clone(),
308307
cxx: CppName::new(ns, ident),
309308
}
310309
}
310+
311+
/// Use this constructor when attributes such as #[rust_name]
312+
/// can be used to potentially give a different name in Rust vs C++.
313+
pub fn new_from_differing_names(ns: Namespace, cxx_ident: Ident, rust_ident: Ident) -> Self {
314+
Self {
315+
rust: rust_ident,
316+
cxx: CppName::new(ns, cxx_ident),
317+
}
318+
}
311319
}
312320

313321
impl ResolvableName {
314322
pub fn new(ident: Ident) -> Self {
315323
Self { rust: ident }
316324
}
317325

318-
pub fn from_pair(pair: Pair) -> Self {
319-
Self { rust: pair.rust }
320-
}
321-
322326
pub fn make_self(span: Span) -> Self {
323327
Self {
324328
rust: Token![Self](span).into(),
@@ -357,9 +361,7 @@ impl CppName {
357361
Self { ns, ident }
358362
}
359363

360-
fn iter_all_segments(
361-
&self,
362-
) -> std::iter::Chain<std::slice::Iter<Ident>, std::iter::Once<&Ident>> {
364+
fn iter_all_segments(&self) -> impl Iterator<Item = &Ident> {
363365
self.ns.iter().chain(std::iter::once(&self.ident))
364366
}
365367

‎syntax/namespace.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
use crate::syntax::qualified::QualifiedName;
2+
#[cfg(test)]
3+
use proc_macro2::Span;
24
use quote::IdentFragment;
35
use std::fmt::{self, Display};
46
use std::slice::Iter;
@@ -36,6 +38,16 @@ impl Namespace {
3638
input.parse::<Option<Token![,]>>()?;
3739
Ok(ns)
3840
}
41+
42+
#[cfg(test)]
43+
pub fn from_str(ns: &str) -> Self {
44+
Namespace {
45+
segments: ns
46+
.split("::")
47+
.map(|x| Ident::new(x, Span::call_site()))
48+
.collect(),
49+
}
50+
}
3951
}
4052

4153
impl Parse for Namespace {

‎syntax/parse.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use crate::syntax::file::{Item, ItemForeignMod};
33
use crate::syntax::report::Errors;
44
use crate::syntax::Atom::*;
55
use crate::syntax::{
6-
attrs, error, Api, CppName, Doc, Enum, ExternFn, ExternType, Impl, Include, IncludeKind, Lang,
6+
attrs, error, Api, Doc, Enum, ExternFn, ExternType, Impl, Include, IncludeKind, Lang,
77
Namespace, Pair, Receiver, Ref, ResolvableName, Signature, Slice, Struct, Ty1, Type, TypeAlias,
88
Var, Variant,
99
};
@@ -245,7 +245,7 @@ fn parse_foreign_mod(
245245
if let Api::CxxFunction(efn) | Api::RustFunction(efn) = item {
246246
if let Some(receiver) = &mut efn.receiver {
247247
if receiver.ty.is_self() {
248-
receiver.ty = ResolvableName::from_pair(single_type.clone());
248+
receiver.ty = ResolvableName::new(single_type.rust.clone());
249249
}
250250
}
251251
}
@@ -398,10 +398,11 @@ fn parse_extern_fn(
398398
let throws = throws_tokens.is_some();
399399
let unsafety = foreign_fn.sig.unsafety;
400400
let fn_token = foreign_fn.sig.fn_token;
401-
let ident = Pair {
402-
cxx: CppName::new(ns, cxx_name.unwrap_or(foreign_fn.sig.ident.clone())),
403-
rust: rust_name.unwrap_or(foreign_fn.sig.ident.clone()),
404-
};
401+
let ident = Pair::new_from_differing_names(
402+
ns,
403+
cxx_name.unwrap_or(foreign_fn.sig.ident.clone()),
404+
rust_name.unwrap_or(foreign_fn.sig.ident.clone()),
405+
);
405406
let paren_token = foreign_fn.sig.paren_token;
406407
let semi_token = foreign_fn.semi_token;
407408
let api_function = match lang {

‎tests/BUCK

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ rust_library(
1515
"ffi/extra.rs",
1616
"ffi/lib.rs",
1717
"ffi/module.rs",
18-
"ffi/class_in_ns.rs",
1918
],
2019
crate = "cxx_test_suite",
2120
deps = [
@@ -31,7 +30,6 @@ cxx_library(
3130
":bridge/source",
3231
":extra/source",
3332
":module/source",
34-
":class_in_ns/source",
3533
],
3634
headers = {
3735
"ffi/lib.rs.h": ":bridge/header",
@@ -54,8 +52,3 @@ rust_cxx_bridge(
5452
name = "module",
5553
src = "ffi/module.rs",
5654
)
57-
58-
rust_cxx_bridge(
59-
name = "class_in_ns",
60-
src = "ffi/class_in_ns.rs",
61-
)

‎tests/BUILD

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ rust_library(
1818
"ffi/extra.rs",
1919
"ffi/lib.rs",
2020
"ffi/module.rs",
21-
"ffi/class_in_ns.rs",
2221
],
2322
deps = [
2423
":impl",
@@ -33,7 +32,6 @@ cc_library(
3332
":bridge/source",
3433
":extra/source",
3534
":module/source",
36-
":class_in_ns/source",
3735
],
3836
hdrs = ["ffi/tests.h"],
3937
deps = [
@@ -59,9 +57,3 @@ rust_cxx_bridge(
5957
src = "ffi/module.rs",
6058
deps = [":impl"],
6159
)
62-
63-
rust_cxx_bridge(
64-
name = "class_in_ns",
65-
src = "ffi/class_in_ns.rs",
66-
deps = [":impl"],
67-
)

‎tests/ffi/build.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ fn main() {
66
}
77

88
CFG.include_prefix = "tests/ffi";
9-
let sources = vec!["lib.rs", "extra.rs", "module.rs", "class_in_ns.rs"];
9+
let sources = vec!["lib.rs", "extra.rs", "module.rs"];
1010
cxx_build::bridges(sources)
1111
.file("tests.cc")
1212
.flag_if_supported(cxxbridge_flags::STD)

‎tests/ffi/class_in_ns.rs

Lines changed: 0 additions & 21 deletions
This file was deleted.

0 commit comments

Comments
 (0)