Various minor tweaks based on a self-review.
PiperOrigin-RevId: 495967727
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index 581a6ae..620c1d4 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -141,7 +141,8 @@
// (processing each `rhs` include one-at-a-time) while `append` steals
// the whole backing data store from `rhs.includes`. OTOH, this is a bit
// speculative, since the (expected / guessed) performance difference is
- // not documented at https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.append
+ // not documented at
+ // https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.append
self.includes.append(&mut rhs.includes);
self.defs.extend(rhs.defs);
@@ -156,7 +157,7 @@
impl CcSnippet {
/// Consumes `self` and returns its `tokens`, while preserving
- /// its `prereqs` into the `prereqs_accumulator` out parameter.
+ /// its `prereqs` into `prereqs_accumulator`.
fn into_tokens(self, prereqs_accumulator: &mut CcPrerequisites) -> TokenStream {
let Self { tokens, prereqs } = self;
*prereqs_accumulator += prereqs;
@@ -176,7 +177,7 @@
}
}
-/// Represents the fully qualified name of a Rust item (e.g. a `struct` or a
+/// Represents the fully qualified name of a Rust item (e.g. of a `struct` or a
/// function).
struct FullyQualifiedName {
/// Name of the crate that defines the item.
@@ -185,10 +186,11 @@
/// Path to the module where the item is located.
/// For example, this would be `cmp` for `std::cmp::Ordering`.
- /// The path can contain multiple modules - e.g. `foo::bar::baz`.
+ /// The path may contain multiple modules - e.g. `foo::bar::baz`.
mod_path: NamespaceQualifier,
/// Name of the item.
+ /// For example, this would be `Ordering` for `std::cmp::Ordering`.
name: Symbol,
}
@@ -222,10 +224,10 @@
}
fn format_for_rs(&self) -> TokenStream {
- let top_level_ns = make_rs_ident(self.krate.as_str());
- let ns_path = self.mod_path.format_for_rs();
+ let krate = make_rs_ident(self.krate.as_str());
+ let mod_path = self.mod_path.format_for_rs();
let name = make_rs_ident(self.name.as_str());
- quote! { :: #top_level_ns :: #ns_path #name }
+ quote! { :: #krate :: #mod_path #name }
}
}
@@ -243,8 +245,8 @@
/// - structs need to be moved: `std::move(value)`
/// - in the future additional processing may be needed for other types (this is
/// speculative so please take these examples with a grain of salt):
-/// - str: utf-8 verification
-/// - &T: calling into `crubit::MutRef::unsafe_get_ptr`
+/// - `&str`: utf-8 verification (see b/262580415)
+/// - `&T`: calling into `crubit::MutRef::unsafe_get_ptr` (see b/258235219)
fn format_cc_thunk_arg<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, value: TokenStream) -> CcSnippet {
if ty.is_copy_modulo_regions(tcx, ty::ParamEnv::empty()) {
CcSnippet::new(value)
@@ -254,7 +256,7 @@
}
/// Formats `ty` into a `CcSnippet` that represents how the type should be
-/// spelled in a C++ declaration of an `extern "C"` function.
+/// spelled in a C++ declaration of a function parameter or field.
//
// TODO(b/259724276): This function's results should be memoized.
fn format_ty_for_cc(tcx: TyCtxt, ty: Ty) -> Result<CcSnippet> {
@@ -272,7 +274,7 @@
ty::TyKind::Tuple(types) => {
if types.len() == 0 {
// TODO(b/254507801): Maybe translate into `crubit::Unit`?
- bail!("The unit type `()` / `void` is only supported as a return type");
+ bail!("`()` / `void` is only supported as a return type (b/254507801)");
} else {
// TODO(b/254099023): Add support for tuples.
bail!("Tuples are not supported yet: {} (b/254099023)", ty);
@@ -340,9 +342,7 @@
}
ty::TyKind::Adt(adt, substs) => {
- if substs.len() != 0 {
- bail!("Generic types are not supported yet (b/259749095)");
- }
+ ensure!(substs.len() == 0, "Generic types are not supported yet (b/259749095)");
// Verify if definition of `ty` can be succesfully imported and bail otherwise.
let def_id = adt.did();
@@ -363,11 +363,11 @@
prereqs
}
},
- // TODO(b/260268230, b/260729464): When recursively processing nested types (e.g. an element type of an
- // Array, a pointee type of a RawPtr, a referent of a Ref or Slice, a parameter type of an
- // FnPtr, etc), one should also 1) propagate `CcPrerequisites::defs`, 2) cover
- // `CcPrerequisites::defs` in `test_format_ty_for_cc...`. For ptr/ref/slice it might be
- // also desirable to separately track forward-declaration prerequisites.
+ // TODO(b/260268230, b/260729464): When recursively processing nested types (e.g. an
+ // element type of an Array, a pointee type of a RawPtr, a referent of a Ref or Slice, a
+ // parameter type of an FnPtr, etc), one should also 1) propagate `CcPrerequisites::defs`,
+ // 2) cover `CcPrerequisites::defs` in `test_format_ty_for_cc...`. For ptr/ref/slice it
+ // might be also desirable to separately track forward-declaration prerequisites.
| ty::TyKind::Array(..)
| ty::TyKind::Slice(..)
| ty::TyKind::RawPtr(..)
@@ -431,10 +431,7 @@
}
}
ty::TyKind::Adt(adt, substs) => {
- if substs.len() != 0 {
- bail!("Generic types are not supported yet (b/259749095)");
- }
-
+ ensure!(substs.len() == 0, "Generic types are not supported yet (b/259749095)");
FullyQualifiedName::new(tcx, adt.did()).format_for_rs()
},
ty::TyKind::Foreign(..)
@@ -464,7 +461,9 @@
})
}
-#[derive(Debug, Default)]
+/// A C++ snippet (e.g. function declaration for `..._cc_api.h`) and a Rust
+/// snippet (e.g. a thunk definition for `..._cc_api_impl.rs`).
+#[derive(Debug)]
struct MixedSnippet {
cc: CcSnippet,
rs: TokenStream,
@@ -544,7 +543,7 @@
}
};
- let FullyQualifiedName { mod_path, name, .. } = FullyQualifiedName::new(tcx, def_id);
+ let FullyQualifiedName { krate, mod_path, name, .. } = FullyQualifiedName::new(tcx, def_id);
let mut cc_prereqs = CcPrerequisites::default();
let cc_tokens = {
@@ -605,7 +604,7 @@
let rs_tokens = if !needs_thunk {
quote! {}
} else {
- let crate_name = make_rs_ident(tcx.crate_name(LOCAL_CRATE).as_str());
+ let crate_name = make_rs_ident(krate.as_str());
let mod_path = mod_path.format_for_rs();
let fn_name = make_rs_ident(name.as_str());
let exported_name = make_rs_ident(symbol_name.name);
@@ -869,11 +868,7 @@
.hir()
.attrs(hir_id)
.iter()
- .filter_map(|attr| match &attr.doc_str() {
- None => None,
- Some(symbol) => Some(symbol.as_str().to_owned()),
- })
- .collect_vec()
+ .filter_map(|attr| attr.doc_str())
.join("\n\n");
if doc_comment.is_empty() {
quote! {}
@@ -2118,7 +2113,7 @@
test_format_def(test_src, "fn_with_params", |result| {
let err = result.expect_err("Test expects an error here");
assert_eq!(err, "Error formatting the type of parameter #0: \
- The unit type `()` / `void` is only supported as a return type");
+ `()` / `void` is only supported as a return type (b/254507801)");
});
}
@@ -2731,7 +2726,7 @@
// ( <Rust type>, <expected error message> )
(
"()", // Empty TyKind::Tuple
- "The unit type `()` / `void` is only supported as a return type"
+ "`()` / `void` is only supported as a return type (b/254507801)"
),
(
// TODO(b/254507801): Expect `crubit::Never` instead (see the bug for more
diff --git a/cc_bindings_from_rs/cc_bindings_from_rs.rs b/cc_bindings_from_rs/cc_bindings_from_rs.rs
index 69b6c92..e6b9764 100644
--- a/cc_bindings_from_rs/cc_bindings_from_rs.rs
+++ b/cc_bindings_from_rs/cc_bindings_from_rs.rs
@@ -33,8 +33,8 @@
cc_tokens_to_formatted_string, rs_tokens_to_formatted_string, RustfmtConfig,
};
-/// This mostly wraps and simplifies a subset of APIs from the `rustc_driver`
-/// module.
+/// The `bindings_driver` module mostly wraps and simplifies a subset of APIs
+/// from the `rustc_driver` module.
mod bindings_driver {
use anyhow::anyhow;
@@ -109,7 +109,7 @@
let rustc_result: anyhow::Result<()> = rustc_result.map_err(|_err| {
// We can ignore `_err` because it has no payload / because this type has only
// one valid/possible value.
- anyhow::format_err!("Errors reported by Rust compiler.")
+ anyhow!("Errors reported by Rust compiler.")
});
// Return either `rustc_result` or `self.callback_result` or a new error.
@@ -160,18 +160,17 @@
}
fn run_with_tcx(cmdline: &Cmdline, tcx: TyCtxt) -> anyhow::Result<()> {
- let bindings = GeneratedBindings::generate(tcx)?;
+ let GeneratedBindings { h_body, rs_body } = GeneratedBindings::generate(tcx)?;
{
- let h_body =
- cc_tokens_to_formatted_string(bindings.h_body, &cmdline.clang_format_exe_path)?;
+ let h_body = cc_tokens_to_formatted_string(h_body, &cmdline.clang_format_exe_path)?;
write_file(&cmdline.h_out, &h_body)?;
}
{
let rustfmt_config =
RustfmtConfig::new(&cmdline.rustfmt_exe_path, cmdline.rustfmt_config_path.as_deref());
- let rs_body = rs_tokens_to_formatted_string(bindings.rs_body, &rustfmt_config)?;
+ let rs_body = rs_tokens_to_formatted_string(rs_body, &rustfmt_config)?;
write_file(&cmdline.rs_out, &rs_body)?;
}
@@ -278,22 +277,14 @@
/// Appends `extra_rustc_args` at the end of the cmdline (i.e. as
/// additional rustc args, in addition to `--sysroot`,
/// `--crate-type=...`, etc.).
- fn with_extra_rustc_args<T>(mut self, extra_rustc_args: T) -> Self
- where
- T: IntoIterator,
- T::Item: Into<String>,
- {
- self.extra_rustc_args = extra_rustc_args.into_iter().map(|t| t.into()).collect_vec();
+ fn with_extra_rustc_args(mut self, extra_rustc_args: &[&str]) -> Self {
+ self.extra_rustc_args = extra_rustc_args.iter().map(|t| t.to_string()).collect_vec();
self
}
/// Appends `extra_crubit_args` before the first `--`.
- fn with_extra_crubit_args<T>(mut self, extra_crubit_args: T) -> Self
- where
- T: IntoIterator,
- T::Item: Into<String>,
- {
- self.extra_crubit_args = extra_crubit_args.into_iter().map(|t| t.into()).collect_vec();
+ fn with_extra_crubit_args(mut self, extra_crubit_args: &[&str]) -> Self {
+ self.extra_crubit_args = extra_crubit_args.iter().map(|t| t.to_string()).collect_vec();
self
}
@@ -443,7 +434,7 @@
// Tests that errors from `Cmdline::new` get propagated. Broader coverage of
// various error types can be found in tests in `cmdline.rs`.
let err = TestArgs::default_args()?
- .with_extra_crubit_args(["--unrecognized-crubit-flag"])
+ .with_extra_crubit_args(&["--unrecognized-crubit-flag"])
.run()
.expect_err("--unrecognized_crubit_flag should trigger an error");
@@ -460,7 +451,7 @@
fn test_rustc_error_propagation() -> anyhow::Result<()> {
// Tests that `rustc` errors are propagated.
let err = TestArgs::default_args()?
- .with_extra_rustc_args(["--unrecognized-rustc-flag"])
+ .with_extra_rustc_args(&["--unrecognized-rustc-flag"])
.run()
.expect_err("--unrecognized-rustc-flag should trigger an error");
@@ -475,7 +466,7 @@
// anything (e.g. when there are no rustc cmdline arguments, or when
// `--help` is present).
let err = TestArgs::default_args()?
- .with_extra_rustc_args(["--help"])
+ .with_extra_rustc_args(&["--help"])
.run()
.expect_err("--help passed to rustc should trigger Crubit-level error");
@@ -517,7 +508,7 @@
let tmpdir = tempdir()?;
let out_path = tmpdir.path().join("unexpected_output.o");
TestArgs::default_args()?
- .with_extra_rustc_args(vec!["-o", &out_path.display().to_string()])
+ .with_extra_rustc_args(&["-o", &out_path.display().to_string()])
.run()
.expect("No rustc or Crubit errors are expected in this test");
diff --git a/cc_bindings_from_rs/cmdline.rs b/cc_bindings_from_rs/cmdline.rs
index 1bddaab..6e961a5 100644
--- a/cc_bindings_from_rs/cmdline.rs
+++ b/cc_bindings_from_rs/cmdline.rs
@@ -97,8 +97,11 @@
assert_eq!(Path::new("foo.h"), cmdline.h_out);
assert_eq!(Path::new("foo_impl.rs"), cmdline.rs_out);
+ assert_eq!(Path::new("clang-format.exe"), cmdline.clang_format_exe_path);
assert_eq!(Path::new("rustfmt.exe"), cmdline.rustfmt_exe_path);
assert!(cmdline.rustfmt_config_path.is_none());
+ // Ignoring `rustc_args` in this test - they are covered in a separate
+ // test below: `test_rustc_args_happy_path`.
}
#[test]
diff --git a/cc_bindings_from_rs/toposort.rs b/cc_bindings_from_rs/toposort.rs
index 263555d..d145bb3 100644
--- a/cc_bindings_from_rs/toposort.rs
+++ b/cc_bindings_from_rs/toposort.rs
@@ -167,7 +167,8 @@
// `ready` contains ids of nodes which have no remaining predecessors (and which
// therefore are ready to be added to the `ordered` result of the
// topological sort). Using a BinaryHeap to store the `ready` nodes helps
- // to extract them in the `preferred_order`. (This is the `S` data structure from https://en.wikipedia.org/wiki/Topological_sorting#Kahn%27s_algorithm.)
+ // to extract them in the `preferred_order`. (This is the `S` data structure from
+ // https://en.wikipedia.org/wiki/Topological_sorting#Kahn%27s_algorithm.)
let mut ready: BinaryHeap<HeapNode<'_, NodeId, CmpFn>> = graph
.iter()
.filter(|(_, graph_node)| graph_node.count_of_predecessors == 0)
@@ -180,7 +181,7 @@
while let Some(HeapNode { id: removed_id, .. }) = ready.pop() {
let removed_graph_node = graph.remove(&removed_id).unwrap();
for succ_id in removed_graph_node.successors.into_iter() {
- let mut succ = graph.get_mut(&succ_id).unwrap();
+ let succ = graph.get_mut(&succ_id).unwrap();
assert!(succ.count_of_predecessors > 0);
succ.count_of_predecessors -= 1;
if succ.count_of_predecessors == 0 {
@@ -223,6 +224,10 @@
successors: Vec<NodeId>,
}
+// TODO(https://github.com/rust-lang/rust/issues/26925): It should be possible to
+// `#[derive(Default)]` here, but `derive` insists that `NodeId` has to
+// implement `Default` even though the default vector doesn't contain any
+// `NodeId`s.
impl<NodeId> Default for GraphNode<NodeId> {
fn default() -> Self {
Self { count_of_predecessors: 0, successors: Vec::new() }
@@ -310,14 +315,14 @@
#[test]
fn test_toposort_deps_compatible_with_preferred_order() {
- let (ordered, failed) = toposort(&[1, 2, 3, 4], &[(2, 3)]);
+ let (ordered, failed) = toposort(&[1, 4, 2, 3], &[(2, 3)]);
assert_eq!(ordered, vec![1, 2, 3, 4]);
assert_eq!(failed, vec![]);
}
#[test]
fn test_toposort_deps_incompatible_with_preferred_order() {
- let (ordered, failed) = toposort(&[1, 2, 3, 4], &[(3, 2)]);
+ let (ordered, failed) = toposort(&[1, 4, 2, 3], &[(3, 2)]);
assert_eq!(ordered, vec![1, 3, 2, 4]);
assert_eq!(failed, vec![]);
}
