Restrict From<S> for {D,Subd}iagnosticMessage.

nnethercote · nnethercote · commit a4c49374cb36 · 2023-05-03T08:44:39.000+10:00
Currently a `{D,Subd}iagnosticMessage` can be created from any type that
impls `Into&lt;String&gt;`. That includes `&amp;str`, `String`, and `Cow&lt;'static,
str&gt;`, which are reasonable. It also includes `&amp;String`, which is pretty
weird, and results in many places making unnecessary allocations for
patterns like this:
```
self.fatal(&amp;format!(...))
```
This creates a string with `format!`, takes a reference, passes the
reference to `fatal`, which does an `into()`, which clones the
reference, doing a second allocation. Two allocations for a single
string, bleh.

This commit changes the `From` impls so that you can only create a
`{D,Subd}iagnosticMessage` from `&amp;str`, `String`, or `Cow&lt;'static,
str&gt;`. This requires changing all the places that currently create one
from a `&amp;String`. Most of these are of the `&amp;format!(...)` form
described above; each one removes an unnecessary static `&amp;`, plus an
allocation when executed. There are also a few places where the existing
use of `&amp;String` was more reasonable; these now just use `clone()` at
the call site.

As well as making the code nicer and more efficient, this is a step
towards possibly using `Cow&lt;'static, str&gt;` in
`{D,Subd}iagnosticMessage::{Str,Eager}`. That would require changing
the `From&lt;&amp;'a str&gt;` impls to `From&lt;&amp;'static str&gt;`, which is doable, but
I'm not yet sure if it's worthwhile.
diff --git a/src/abi/mod.rs b/src/abi/mod.rs
@@ -88,10 +88,10 @@ pub(crate) fn import_function<'tcx>(
     let sig = get_function_sig(tcx, module.target_config().default_call_conv, inst);
     match module.declare_function(name, Linkage::Import, &sig) {
         Ok(func_id) => func_id,
-        Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(&format!(
+        Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(format!(
             "attempt to declare `{name}` as function, but it was already declared as static"
         )),
-        Err(ModuleError::IncompatibleSignature(_, prev_sig, new_sig)) => tcx.sess.fatal(&format!(
+        Err(ModuleError::IncompatibleSignature(_, prev_sig, new_sig)) => tcx.sess.fatal(format!(
             "attempt to declare `{name}` with signature {new_sig:?}, \
              but it was already declared with signature {prev_sig:?}"
         )),
@@ -548,7 +548,7 @@ pub(crate) fn codegen_terminator_call<'tcx>(
             if !matches!(fn_sig.abi(), Abi::C { .. }) {
                 fx.tcx.sess.span_fatal(
                     source_info.span,
-                    &format!("Variadic call for non-C abi {:?}", fn_sig.abi()),
+                    format!("Variadic call for non-C abi {:?}", fn_sig.abi()),
                 );
             }
             let sig_ref = fx.bcx.func.dfg.call_signature(call_inst).unwrap();
@@ -560,7 +560,7 @@ pub(crate) fn codegen_terminator_call<'tcx>(
                         // FIXME set %al to upperbound on float args once floats are supported
                         fx.tcx.sess.span_fatal(
                             source_info.span,
-                            &format!("Non int ty {:?} for variadic call", ty),
+                            format!("Non int ty {:?} for variadic call", ty),
                         );
                     }
                     AbiParam::new(ty)
diff --git a/src/base.rs b/src/base.rs
@@ -220,13 +220,13 @@ pub(crate) fn verify_func(
         match cranelift_codegen::verify_function(&func, &flags) {
             Ok(_) => {}
             Err(err) => {
-                tcx.sess.err(&format!("{:?}", err));
+                tcx.sess.err(format!("{:?}", err));
                 let pretty_error = cranelift_codegen::print_errors::pretty_verifier_error(
                     &func,
                     Some(Box::new(writer)),
                     err,
                 );
-                tcx.sess.fatal(&format!("cranelift verify error:\n{}", pretty_error));
+                tcx.sess.fatal(format!("cranelift verify error:\n{}", pretty_error));
             }
         }
     });
diff --git a/src/common.rs b/src/common.rs
@@ -481,7 +481,7 @@ impl<'tcx> LayoutOfHelpers<'tcx> for RevealAllLayoutCx<'tcx> {
     #[inline]
     fn handle_layout_err(&self, err: LayoutError<'tcx>, span: Span, ty: Ty<'tcx>) -> ! {
         if let layout::LayoutError::SizeOverflow(_) = err {
-            self.0.sess.span_fatal(span, &err.to_string())
+            self.0.sess.span_fatal(span, err.to_string())
         } else {
             span_bug!(span, "failed to get layout for `{}`: {}", ty, err)
         }
@@ -499,7 +499,7 @@ impl<'tcx> FnAbiOfHelpers<'tcx> for RevealAllLayoutCx<'tcx> {
         fn_abi_request: FnAbiRequest<'tcx>,
     ) -> ! {
         if let FnAbiError::Layout(LayoutError::SizeOverflow(_)) = err {
-            self.0.sess.span_fatal(span, &err.to_string())
+            self.0.sess.span_fatal(span, err.to_string())
         } else {
             match fn_abi_request {
                 FnAbiRequest::OfFnPtr { sig, extra_args } => {
diff --git a/src/concurrency_limiter.rs b/src/concurrency_limiter.rs
@@ -65,7 +65,7 @@ impl ConcurrencyLimiter {
                     // Make sure to drop the mutex guard first to prevent poisoning the mutex.
                     drop(state);
                     if let Some(err) = err {
-                        handler.fatal(&err).raise();
+                        handler.fatal(err).raise();
                     } else {
                         // The error was already emitted, but compilation continued. Raise a silent
                         // fatal error.
diff --git a/src/constant.rs b/src/constant.rs
@@ -308,7 +308,7 @@ fn data_id_for_static(
             attrs.flags.contains(CodegenFnAttrFlags::THREAD_LOCAL),
         ) {
             Ok(data_id) => data_id,
-            Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(&format!(
+            Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(format!(
                 "attempt to declare `{symbol_name}` as static, but it was already declared as function"
             )),
             Err(err) => Err::<_, _>(err).unwrap(),
@@ -356,7 +356,7 @@ fn data_id_for_static(
         attrs.flags.contains(CodegenFnAttrFlags::THREAD_LOCAL),
     ) {
         Ok(data_id) => data_id,
-        Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(&format!(
+        Err(ModuleError::IncompatibleDeclaration(_)) => tcx.sess.fatal(format!(
             "attempt to declare `{symbol_name}` as static, but it was already declared as function"
         )),
         Err(err) => Err::<_, _>(err).unwrap(),
@@ -404,7 +404,7 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
                 if let Some(names) = section_name.split_once(',') {
                     names
                 } else {
-                    tcx.sess.fatal(&format!(
+                    tcx.sess.fatal(format!(
                         "#[link_section = \"{}\"] is not valid for macos target: must be segment and section separated by comma",
                         section_name
                     ));
@@ -449,7 +449,7 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
                 GlobalAlloc::Static(def_id) => {
                     if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::THREAD_LOCAL)
                     {
-                        tcx.sess.fatal(&format!(
+                        tcx.sess.fatal(format!(
                             "Allocation {:?} contains reference to TLS value {:?}",
                             alloc_id, def_id
                         ));
diff --git a/src/driver/aot.rs b/src/driver/aot.rs
@@ -69,7 +69,7 @@ impl OngoingCodegen {
 
             let module_codegen_result = match module_codegen_result {
                 Ok(module_codegen_result) => module_codegen_result,
-                Err(err) => sess.fatal(&err),
+                Err(err) => sess.fatal(err),
             };
             let ModuleCodegenResult { module_regular, module_global_asm, existing_work_product } =
                 module_codegen_result;
@@ -468,7 +468,7 @@ pub(crate) fn run_aot(
             let obj = create_compressed_metadata_file(tcx.sess, &metadata, &symbol_name);
 
             if let Err(err) = std::fs::write(&tmp_file, obj) {
-                tcx.sess.fatal(&format!("error writing metadata object file: {}", err));
+                tcx.sess.fatal(format!("error writing metadata object file: {}", err));
             }
 
             (metadata_cgu_name, tmp_file)
diff --git a/src/intrinsics/llvm.rs b/src/intrinsics/llvm.rs
@@ -42,7 +42,7 @@ pub(crate) fn codegen_llvm_intrinsic_call<'tcx>(
         _ => {
             fx.tcx
                 .sess
-                .warn(&format!("unsupported llvm intrinsic {}; replacing with trap", intrinsic));
+                .warn(format!("unsupported llvm intrinsic {}; replacing with trap", intrinsic));
             crate::trap::trap_unimplemented(fx, intrinsic);
             return;
         }
diff --git a/src/intrinsics/llvm_aarch64.rs b/src/intrinsics/llvm_aarch64.rs
@@ -207,7 +207,7 @@ pub(crate) fn codegen_aarch64_llvm_intrinsic_call<'tcx>(
         }
         */
         _ => {
-            fx.tcx.sess.warn(&format!(
+            fx.tcx.sess.warn(format!(
                 "unsupported AArch64 llvm intrinsic {}; replacing with trap",
                 intrinsic
             ));
diff --git a/src/intrinsics/llvm_x86.rs b/src/intrinsics/llvm_x86.rs
@@ -138,10 +138,9 @@ pub(crate) fn codegen_x86_llvm_intrinsic_call<'tcx>(
             llvm_add_sub(fx, BinOp::Sub, ret, b_in, a, b);
         }
         _ => {
-            fx.tcx.sess.warn(&format!(
-                "unsupported x86 llvm intrinsic {}; replacing with trap",
-                intrinsic
-            ));
+            fx.tcx
+                .sess
+                .warn(format!("unsupported x86 llvm intrinsic {}; replacing with trap", intrinsic));
             crate::trap::trap_unimplemented(fx, intrinsic);
             return;
         }
diff --git a/src/intrinsics/mod.rs b/src/intrinsics/mod.rs
@@ -42,7 +42,7 @@ fn report_atomic_type_validation_error<'tcx>(
 ) {
     fx.tcx.sess.span_err(
         span,
-        &format!(
+        format!(
             "`{}` intrinsic: expected basic integer or raw pointer type, found `{:?}`",
             intrinsic, ty
         ),
@@ -1202,7 +1202,7 @@ fn codegen_regular_intrinsic_call<'tcx>(
         _ => {
             fx.tcx
                 .sess
-                .span_fatal(source_info.span, &format!("unsupported intrinsic {}", intrinsic));
+                .span_fatal(source_info.span, format!("unsupported intrinsic {}", intrinsic));
         }
     }
 
diff --git a/src/intrinsics/simd.rs b/src/intrinsics/simd.rs
@@ -13,7 +13,7 @@ fn report_simd_type_validation_error(
     span: Span,
     ty: Ty<'_>,
 ) {
-    fx.tcx.sess.span_err(span, &format!("invalid monomorphization of `{}` intrinsic: expected SIMD input type, found non-SIMD `{}`", intrinsic, ty));
+    fx.tcx.sess.span_err(span, format!("invalid monomorphization of `{}` intrinsic: expected SIMD input type, found non-SIMD `{}`", intrinsic, ty));
     // Prevent verifier error
     fx.bcx.ins().trap(TrapCode::UnreachableCodeReached);
 }
@@ -150,7 +150,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
                     _ => {
                         fx.tcx.sess.span_err(
                             span,
-                            &format!(
+                            format!(
                                 "simd_shuffle index must be an array of `u32`, got `{}`",
                                 idx_ty,
                             ),
@@ -248,7 +248,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
             if idx >= lane_count.into() {
                 fx.tcx.sess.span_fatal(
                     fx.mir.span,
-                    &format!("[simd_insert] idx {} >= lane_count {}", idx, lane_count),
+                    format!("[simd_insert] idx {} >= lane_count {}", idx, lane_count),
                 );
             }
 
@@ -296,7 +296,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
             if idx >= lane_count.into() {
                 fx.tcx.sess.span_fatal(
                     fx.mir.span,
-                    &format!("[simd_extract] idx {} >= lane_count {}", idx, lane_count),
+                    format!("[simd_extract] idx {} >= lane_count {}", idx, lane_count),
                 );
             }
 
@@ -699,7 +699,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
                 _ => {
                     fx.tcx.sess.span_fatal(
                         span,
-                        &format!(
+                        format!(
                             "invalid monomorphization of `simd_bitmask` intrinsic: \
                             vector argument `{}`'s element type `{}`, expected integer element \
                             type",
@@ -739,7 +739,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
                 _ => {
                     fx.tcx.sess.span_fatal(
                         span,
-                        &format!(
+                        format!(
                             "invalid monomorphization of `simd_bitmask` intrinsic: \
                             cannot return `{}`, expected `u{}` or `[u8; {}]`",
                             ret.layout().ty,
@@ -875,7 +875,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
         }
 
         _ => {
-            fx.tcx.sess.span_err(span, &format!("Unknown SIMD intrinsic {}", intrinsic));
+            fx.tcx.sess.span_err(span, format!("Unknown SIMD intrinsic {}", intrinsic));
             // Prevent verifier error
             fx.bcx.ins().trap(TrapCode::UnreachableCodeReached);
         }
diff --git a/src/lib.rs b/src/lib.rs
@@ -185,7 +185,7 @@ impl CodegenBackend for CraneliftCodegenBackend {
         let mut config = self.config.borrow_mut();
         if config.is_none() {
             let new_config = BackendConfig::from_opts(&sess.opts.cg.llvm_args)
-                .unwrap_or_else(|err| sess.fatal(&err));
+                .unwrap_or_else(|err| sess.fatal(err));
             *config = Some(new_config);
         }
     }
@@ -245,7 +245,7 @@ impl CodegenBackend for CraneliftCodegenBackend {
 fn target_triple(sess: &Session) -> target_lexicon::Triple {
     match sess.target.llvm_target.parse() {
         Ok(triple) => triple,
-        Err(err) => sess.fatal(&format!("target not recognized: {}", err)),
+        Err(err) => sess.fatal(format!("target not recognized: {}", err)),
     }
 }
 
@@ -307,7 +307,7 @@ fn build_isa(sess: &Session, backend_config: &BackendConfig) -> Arc<dyn isa::Tar
         Some(value) => {
             let mut builder =
                 cranelift_codegen::isa::lookup(target_triple.clone()).unwrap_or_else(|err| {
-                    sess.fatal(&format!("can't compile for {}: {}", target_triple, err));
+                    sess.fatal(format!("can't compile for {}: {}", target_triple, err));
                 });
             if let Err(_) = builder.enable(value) {
                 sess.fatal("the specified target cpu isn't currently supported by Cranelift.");
@@ -317,7 +317,7 @@ fn build_isa(sess: &Session, backend_config: &BackendConfig) -> Arc<dyn isa::Tar
         None => {
             let mut builder =
                 cranelift_codegen::isa::lookup(target_triple.clone()).unwrap_or_else(|err| {
-                    sess.fatal(&format!("can't compile for {}: {}", target_triple, err));
+                    sess.fatal(format!("can't compile for {}: {}", target_triple, err));
                 });
             if target_triple.architecture == target_lexicon::Architecture::X86_64 {
                 // Don't use "haswell" as the default, as it implies `has_lzcnt`.
@@ -330,7 +330,7 @@ fn build_isa(sess: &Session, backend_config: &BackendConfig) -> Arc<dyn isa::Tar
 
     match isa_builder.finish(flags) {
         Ok(target_isa) => target_isa,
-        Err(err) => sess.fatal(&format!("failed to build TargetIsa: {}", err)),
+        Err(err) => sess.fatal(format!("failed to build TargetIsa: {}", err)),
     }
 }
 
diff --git a/src/main_shim.rs b/src/main_shim.rs
@@ -75,7 +75,7 @@ pub(crate) fn maybe_create_entry_wrapper(
             Ok(func_id) => func_id,
             Err(err) => {
                 tcx.sess
-                    .fatal(&format!("entry symbol `{entry_name}` declared multiple times: {err}"));
+                    .fatal(format!("entry symbol `{entry_name}` declared multiple times: {err}"));
             }
         };
 
@@ -171,7 +171,7 @@ pub(crate) fn maybe_create_entry_wrapper(
         }
 
         if let Err(err) = m.define_function(cmain_func_id, &mut ctx) {
-            tcx.sess.fatal(&format!("entry symbol `{entry_name}` defined multiple times: {err}"));
+            tcx.sess.fatal(format!("entry symbol `{entry_name}` defined multiple times: {err}"));
         }
 
         unwind_context.add_function(cmain_func_id, &ctx, m.isa());
diff --git a/src/value_and_place.rs b/src/value_and_place.rs
@@ -344,7 +344,7 @@ impl<'tcx> CPlace<'tcx> {
         if layout.size.bytes() >= u64::from(u32::MAX - 16) {
             fx.tcx
                 .sess
-                .fatal(&format!("values of type {} are too big to store on the stack", layout.ty));
+                .fatal(format!("values of type {} are too big to store on the stack", layout.ty));
         }
 
         let stack_slot = fx.bcx.create_sized_stack_slot(StackSlotData {

Original file line number	Diff line number	Diff line change
`@@ -42,7 +42,7 @@ pub(crate) fn codegen_llvm_intrinsic_call<'tcx>(`
`42`	`42`	`_ => {`
`43`	`43`	`fx.tcx`
`44`	`44`	`.sess`
`45`		`- .warn(&format!("unsupported llvm intrinsic {}; replacing with trap", intrinsic));`
	`45`	`+ .warn(format!("unsupported llvm intrinsic {}; replacing with trap", intrinsic));`
`46`	`46`	`crate::trap::trap_unimplemented(fx, intrinsic);`
`47`	`47`	`return;`
`48`	`48`	`}`
Original file line number	Diff line number	Diff line change
`@@ -207,7 +207,7 @@ pub(crate) fn codegen_aarch64_llvm_intrinsic_call<'tcx>(`
`207`	`207`	`}`
`208`	`208`	`*/`
`209`	`209`	`_ => {`
`210`		`- fx.tcx.sess.warn(&format!(`
	`210`	`+ fx.tcx.sess.warn(format!(`
`211`	`211`	`"unsupported AArch64 llvm intrinsic {}; replacing with trap",`
`212`	`212`	`intrinsic`
`213`	`213`	`));`
Original file line number	Diff line number	Diff line change
`@@ -138,10 +138,9 @@ pub(crate) fn codegen_x86_llvm_intrinsic_call<'tcx>(`
`138`	`138`	`llvm_add_sub(fx, BinOp::Sub, ret, b_in, a, b);`
`139`	`139`	`}`
`140`	`140`	`_ => {`
`141`		`- fx.tcx.sess.warn(&format!(`
`142`		`- "unsupported x86 llvm intrinsic {}; replacing with trap",`
`143`		`- intrinsic`
`144`		`- ));`
	`141`	`+ fx.tcx`
	`142`	`+ .sess`
	`143`	`+ .warn(format!("unsupported x86 llvm intrinsic {}; replacing with trap", intrinsic));`
`145`	`144`	`crate::trap::trap_unimplemented(fx, intrinsic);`
`146`	`145`	`return;`
`147`	`146`	`}`