Skip to content

Commit

Permalink
Avoid some format! into immediate format! (#2913)
Browse files Browse the repository at this point in the history
# Objective

- Avoid usages of `format!` that ~immediately get passed to another `format!`. This avoids a temporary allocation and is just generally cleaner.

## Solution

- `bevy_derive::shader_defs` does a `format!("{}", val.to_string())`, which is better written as just `format!("{}", val)`
- `bevy_diagnostic::log_diagnostics_plugin` does a `format!("{:>}", format!(...))`, which is better written as `format!("{:>}", format_args!(...))`
- `bevy_ecs::schedule` does `tracing::info!(..., name = &*format!("{:?}", val))`, which is better written with the tracing shorthand `tracing::info!(..., name = ?val)`
- `bevy_reflect::reflect` does `f.write_str(&format!(...))`, which is better written as `write!(f, ...)` (this could also be written using `f.debug_tuple`, but I opted to maintain alt debug behavior)
- `bevy_reflect::serde::{ser, de}` do `serde::Error::custom(format!(...))`, which is better written as `Error::custom(format_args!(...))`, as `Error::custom` takes `impl Display` and just immediately calls `format!` again
  • Loading branch information
CAD97 committed Oct 6, 2021
1 parent 07ed1d0 commit a60fe30
Show file tree
Hide file tree
Showing 6 changed files with 19 additions and 15 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_derive/src/shader_defs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub fn derive_shader_defs(input: TokenStream) -> TokenStream {
let struct_name_pascal_case = ast.ident.to_string().to_pascal_case();
let shader_defs = shader_def_idents
.iter()
.map(|i| format!("{}_{}", struct_name_pascal_case, i.to_string()).to_uppercase());
.map(|i| format!("{}_{}", struct_name_pascal_case, i).to_uppercase());

let shader_defs_len = shader_defs.len();
let shader_def_indices = 0..shader_defs_len;
Expand Down
18 changes: 10 additions & 8 deletions crates/bevy_diagnostic/src/log_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,24 @@ impl LogDiagnosticsPlugin {
if let Some(average) = diagnostic.average() {
info!(
target: "bevy diagnostic",
"{:<name_width$}: {:>12} (avg {:>})",
diagnostic.name,
// Suffix is only used for 's' as in seconds currently,
// so we reserve one column for it
format!("{:.6}{:1}", value, diagnostic.suffix),
// so we reserve one column for it; however,
// Do not reserve one column for the suffix in the average
// The ) hugging the value is more aesthetically pleasing
format!("{:.6}{:}", average, diagnostic.suffix),
"{name:<name_width$}: {value:>11.6}{suffix:1} (avg {average:>.6}{suffix:})",
name = diagnostic.name,
value = value,
suffix = diagnostic.suffix,
average = average,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
);
} else {
info!(
target: "bevy diagnostic",
"{:<name_width$}: {:>}",
diagnostic.name,
format!("{:.6}{:}", value, diagnostic.suffix),
"{name:<name_width$}: {value:>.6}{suffix:}",
name = diagnostic.name,
value = value,
suffix = diagnostic.suffix,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
);
}
Expand Down
3 changes: 1 addition & 2 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,7 @@ impl Schedule {
pub fn run_once(&mut self, world: &mut World) {
for label in self.stage_order.iter() {
#[cfg(feature = "trace")]
let stage_span =
bevy_utils::tracing::info_span!("stage", name = &format!("{:?}", label) as &str);
let stage_span = bevy_utils::tracing::info_span!("stage", name = ?label);
#[cfg(feature = "trace")]
let _stage_guard = stage_span.enter();
let stage = self.stages.get_mut(label).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_reflect/src/reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub unsafe trait Reflect: Any + Send + Sync {

impl Debug for dyn Reflect {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(&format!("Reflect({})", self.type_name()))
write!(f, "Reflect({})", self.type_name())
}
}

Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_reflect/src/serde/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,14 @@ impl<'a, 'de> Visitor<'de> for ReflectVisitor<'a> {
.ok_or_else(|| de::Error::missing_field(type_fields::TYPE))?;
let registration =
self.registry.get_with_name(&type_name).ok_or_else(|| {
de::Error::custom(format!("No registration found for {}", type_name))
de::Error::custom(format_args!(
"No registration found for {}",
type_name
))
})?;
let deserialize_reflect =
registration.data::<ReflectDeserialize>().ok_or_else(|| {
de::Error::custom(format!(
de::Error::custom(format_args!(
"The TypeRegistration for {} doesn't have DeserializeReflect",
type_name
))
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_reflect/src/serde/ser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ impl<'a> Serializable<'a> {

fn get_serializable<E: serde::ser::Error>(reflect_value: &dyn Reflect) -> Result<Serializable, E> {
reflect_value.serializable().ok_or_else(|| {
serde::ser::Error::custom(&format!(
serde::ser::Error::custom(format_args!(
"Type '{}' does not support ReflectValue serialization",
reflect_value.type_name()
))
Expand Down

0 comments on commit a60fe30

Please sign in to comment.