From 75ce54f187091c4a5c041521382fe52a29a10e3c Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 3 Nov 2025 11:58:45 +0800 Subject: [PATCH 1/4] refactor: review record structure design Signed-off-by: tison --- CHANGELOG.md | 1 + bridges/log/src/lib.rs | 6 +- core/src/filter/env_filter/mod.rs | 8 +- core/src/filter/env_filter/tests.rs | 7 +- core/src/filter/mod.rs | 15 ++-- core/src/logger/log_impl.rs | 10 +-- core/src/record.rs | 103 +++++++++------------- logforth/examples/custom_layout_filter.rs | 6 +- 8 files changed, 70 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ce2375..a2574a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. ### Breaking changes * Rename `DefaultTrap` to `BestEffortTrap` for better clarity. +* Rename `logforth_core::record::Metadata` to `FilterCriteria`. * Redesign `LevelFilter` to allow different comparison methods. * Redesign `Level` as opentelemetry severity numbers. * Add `Level::Fatal` variant to represent fatal level logs. diff --git a/bridges/log/src/lib.rs b/bridges/log/src/lib.rs index 39b6055..7680799 100644 --- a/bridges/log/src/lib.rs +++ b/bridges/log/src/lib.rs @@ -22,7 +22,7 @@ use logforth_core::Logger; use logforth_core::default_logger; use logforth_core::kv::Key; use logforth_core::kv::Value; -use logforth_core::record::MetadataBuilder; +use logforth_core::record::FilterCriteria; use logforth_core::record::RecordBuilder; fn level_to_level(level: log::Level) -> logforth_core::record::Level { @@ -100,12 +100,12 @@ impl log::Log for OwnedLogProxy { } fn forward_enabled(logger: &Logger, metadata: &Metadata) -> bool { - let metadata = MetadataBuilder::default() + let criteria = FilterCriteria::builder() .target(metadata.target()) .level(level_to_level(metadata.level())) .build(); - Logger::enabled(logger, &metadata) + Logger::enabled(logger, &criteria) } fn forward_log(logger: &Logger, record: &Record) { diff --git a/core/src/filter/env_filter/mod.rs b/core/src/filter/env_filter/mod.rs index 0dccef6..5edcbe6 100644 --- a/core/src/filter/env_filter/mod.rs +++ b/core/src/filter/env_filter/mod.rs @@ -75,9 +75,9 @@ use crate::Diagnostic; use crate::Error; use crate::Filter; use crate::filter::FilterResult; +use crate::record::FilterCriteria; use crate::record::Level; use crate::record::LevelFilter; -use crate::record::Metadata; #[cfg(test)] mod tests; @@ -110,9 +110,9 @@ impl EnvFilter { } impl Filter for EnvFilter { - fn enabled(&self, metadata: &Metadata, _: &[Box]) -> FilterResult { - let level = metadata.level(); - let target = metadata.target(); + fn enabled(&self, criteria: &FilterCriteria, _: &[Box]) -> FilterResult { + let level = criteria.level(); + let target = criteria.target(); // search for the longest match, the vector is assumed to be pre-sorted for directive in self.directives.iter().rev() { diff --git a/core/src/filter/env_filter/tests.rs b/core/src/filter/env_filter/tests.rs index e04749f..0a2c514 100644 --- a/core/src/filter/env_filter/tests.rs +++ b/core/src/filter/env_filter/tests.rs @@ -21,17 +21,18 @@ use crate::filter::env_filter::Directive; use crate::filter::env_filter::EnvFilterBuilder; use crate::filter::env_filter::ParseResult; use crate::filter::env_filter::parse_spec; +use crate::record::FilterCriteria; use crate::record::Level; use crate::record::LevelFilter; -use crate::record::MetadataBuilder; impl EnvFilter { fn rejected(&self, level: Level, target: &str) -> bool { - let metadata = MetadataBuilder::default() + let criteria = FilterCriteria::builder() .level(level) .target(target) .build(); - matches!(Filter::enabled(self, &metadata, &[]), FilterResult::Reject) + + matches!(Filter::enabled(self, &criteria, &[]), FilterResult::Reject) } } diff --git a/core/src/filter/mod.rs b/core/src/filter/mod.rs index 2d6e0bd..2bbd212 100644 --- a/core/src/filter/mod.rs +++ b/core/src/filter/mod.rs @@ -17,8 +17,8 @@ use std::fmt; use crate::Diagnostic; +use crate::record::FilterCriteria; use crate::record::LevelFilter; -use crate::record::Metadata; use crate::record::Record; pub mod env_filter; @@ -39,17 +39,22 @@ pub enum FilterResult { /// A filter that can be applied to log records. pub trait Filter: fmt::Debug + Send + Sync + 'static { /// Whether the record is filtered by its given metadata. - fn enabled(&self, metadata: &Metadata, diags: &[Box]) -> FilterResult; + fn enabled(&self, criteria: &FilterCriteria, diags: &[Box]) -> FilterResult; /// Whether the record is filtered. fn matches(&self, record: &Record, diags: &[Box]) -> FilterResult { - self.enabled(record.metadata(), diags) + let criteria = FilterCriteria::builder() + .level(record.level()) + .target(record.target()) + .build(); + + self.enabled(&criteria, diags) } } impl Filter for LevelFilter { - fn enabled(&self, metadata: &Metadata, _: &[Box]) -> FilterResult { - if self.test(metadata.level()) { + fn enabled(&self, criteria: &FilterCriteria, _: &[Box]) -> FilterResult { + if self.test(criteria.level()) { FilterResult::Neutral } else { FilterResult::Reject diff --git a/core/src/logger/log_impl.rs b/core/src/logger/log_impl.rs index e509c54..9e9a336 100644 --- a/core/src/logger/log_impl.rs +++ b/core/src/logger/log_impl.rs @@ -22,7 +22,7 @@ use crate::Diagnostic; use crate::Error; use crate::Filter; use crate::filter::FilterResult; -use crate::record::Metadata; +use crate::record::FilterCriteria; use crate::record::Record; static DEFAULT_LOGGER: OnceLock = OnceLock::new(); @@ -99,10 +99,10 @@ impl Logger { impl Logger { /// Determine if a log message with the specified metadata would be logged. - pub fn enabled(&self, metadata: &Metadata) -> bool { + pub fn enabled(&self, criteria: &FilterCriteria) -> bool { self.dispatches .iter() - .any(|dispatch| dispatch.enabled(metadata)) + .any(|dispatch| dispatch.enabled(criteria)) } /// Log the [`Record`]. @@ -165,11 +165,11 @@ impl Dispatch { } } - fn enabled(&self, metadata: &Metadata) -> bool { + fn enabled(&self, criteria: &FilterCriteria) -> bool { let diagnostics = &self.diagnostics; for filter in &self.filters { - match filter.enabled(metadata, diagnostics) { + match filter.enabled(criteria, diagnostics) { FilterResult::Reject => return false, FilterResult::Accept => return true, FilterResult::Neutral => {} diff --git a/core/src/record.rs b/core/src/record.rs index e2a4215..b7a6f25 100644 --- a/core/src/record.rs +++ b/core/src/record.rs @@ -63,7 +63,8 @@ pub struct Record<'a> { now: SystemTime, // the metadata - metadata: Metadata<'a>, + level: Level, + target: &'a str, module_path: Option>, file: Option>, line: Option, @@ -81,19 +82,14 @@ impl<'a> Record<'a> { self.now } - /// Metadata about the log directive. - pub fn metadata(&self) -> &Metadata<'a> { - &self.metadata - } - /// The verbosity level of the message. pub fn level(&self) -> Level { - self.metadata.level() + self.level } /// The name of the target of the directive. pub fn target(&self) -> &'a str { - self.metadata.target() + self.target } /// The module path of the message. @@ -151,10 +147,8 @@ impl<'a> Record<'a> { pub fn to_owned(&self) -> RecordOwned { RecordOwned { now: self.now, - metadata: MetadataOwned { - level: self.metadata.level, - target: Str::new_shared(self.metadata.target), - }, + level: self.level, + target: Str::new_shared(self.target), module_path: self.module_path.map(MaybeStaticStr::into_str), file: self.file.map(MaybeStaticStr::into_str), line: self.line, @@ -172,10 +166,8 @@ impl<'a> Record<'a> { RecordBuilder { record: Record { now: self.now, - metadata: Metadata { - level: self.metadata.level, - target: self.metadata.target, - }, + level: self.level, + target: self.target, module_path: self.module_path, file: self.file, line: self.line, @@ -202,7 +194,8 @@ impl Default for RecordBuilder<'_> { RecordBuilder { record: Record { now: SystemTime::now(), - metadata: MetadataBuilder::default().build(), + level: Level::Info, + target: "", module_path: None, file: None, line: None, @@ -223,23 +216,15 @@ impl<'a> RecordBuilder<'a> { self } - /// Set [`metadata`](Record::metadata). - /// - /// Construct a `Metadata` object with [`MetadataBuilder`]. - pub fn metadata(mut self, metadata: Metadata<'a>) -> Self { - self.record.metadata = metadata; - self - } - - /// Set [`Metadata::level`]. + /// Set [`level`](Record::level). pub fn level(mut self, level: Level) -> Self { - self.record.metadata.level = level; + self.record.level = level; self } - /// Set [`Metadata::target`]. + /// Set [`target`](Record::target). pub fn target(mut self, target: &'a str) -> Self { - self.record.metadata.target = target; + self.record.target = target; self } @@ -285,50 +270,50 @@ impl<'a> RecordBuilder<'a> { } } -/// Metadata about a log message. +/// A minimal set of criteria for pre-filtering purposes. #[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] -pub struct Metadata<'a> { +pub struct FilterCriteria<'a> { level: Level, target: &'a str, } -impl<'a> Metadata<'a> { - /// Get the level. +impl<'a> FilterCriteria<'a> { + /// Get the [`level`](Record::level). pub fn level(&self) -> Level { self.level } - /// Get the target. + /// Get the [`target`](Record::target). pub fn target(&self) -> &'a str { self.target } - /// Create a builder initialized with the current metadata's values. - pub fn to_builder(&self) -> MetadataBuilder<'a> { - MetadataBuilder { - metadata: Metadata { + /// Create a builder initialized with the current criteria's values. + pub fn to_builder(&self) -> FilterCriteriaBuilder<'a> { + FilterCriteriaBuilder { + metadata: FilterCriteria { level: self.level, target: self.target, }, } } - /// Returns a new builder. - pub fn builder() -> MetadataBuilder<'a> { - MetadataBuilder::default() + /// Return a brand-new builder. + pub fn builder() -> FilterCriteriaBuilder<'a> { + FilterCriteriaBuilder::default() } } -/// Builder for [`Metadata`]. +/// Builder for [`FilterCriteria`]. #[derive(Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] -pub struct MetadataBuilder<'a> { - metadata: Metadata<'a>, +pub struct FilterCriteriaBuilder<'a> { + metadata: FilterCriteria<'a>, } -impl Default for MetadataBuilder<'_> { +impl Default for FilterCriteriaBuilder<'_> { fn default() -> Self { - MetadataBuilder { - metadata: Metadata { + FilterCriteriaBuilder { + metadata: FilterCriteria { level: Level::Info, target: Default::default(), }, @@ -336,21 +321,21 @@ impl Default for MetadataBuilder<'_> { } } -impl<'a> MetadataBuilder<'a> { - /// Setter for [`level`](Metadata::level). +impl<'a> FilterCriteriaBuilder<'a> { + /// Setter for [`level`](FilterCriteria::level). pub fn level(mut self, arg: Level) -> Self { self.metadata.level = arg; self } - /// Setter for [`target`](Metadata::target). + /// Setter for [`target`](FilterCriteria::target). pub fn target(mut self, target: &'a str) -> Self { self.metadata.target = target; self } /// Invoke the builder and return a `Metadata` - pub fn build(self) -> Metadata<'a> { + pub fn build(self) -> FilterCriteria<'a> { self.metadata } } @@ -362,7 +347,8 @@ pub struct RecordOwned { now: SystemTime, // the metadata - metadata: MetadataOwned, + level: Level, + target: Str<'static>, module_path: Option>, file: Option>, line: Option, @@ -374,22 +360,13 @@ pub struct RecordOwned { kvs: Vec<(kv::KeyOwned, kv::ValueOwned)>, } -/// Owned version of metadata about a log message. -#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] -struct MetadataOwned { - level: Level, - target: Str<'static>, -} - impl RecordOwned { /// Create a `Record` referencing the data in this `RecordOwned`. pub fn as_record(&self) -> Record<'_> { Record { now: self.now, - metadata: Metadata { - level: self.metadata.level, - target: &self.metadata.target, - }, + level: self.level, + target: self.target.get(), module_path: self.module_path.as_deref().map(MaybeStaticStr::Str), file: self.file.as_deref().map(MaybeStaticStr::Str), line: self.line, diff --git a/logforth/examples/custom_layout_filter.rs b/logforth/examples/custom_layout_filter.rs index 18ef56b..fe7c12e 100644 --- a/logforth/examples/custom_layout_filter.rs +++ b/logforth/examples/custom_layout_filter.rs @@ -20,16 +20,16 @@ use logforth::Filter; use logforth::Layout; use logforth::append; use logforth::filter::FilterResult; +use logforth::record::FilterCriteria; use logforth::record::Level; -use logforth::record::Metadata; use logforth::record::Record; #[derive(Debug)] struct CustomFilter; impl Filter for CustomFilter { - fn enabled(&self, metadata: &Metadata, _: &[Box]) -> FilterResult { - if metadata.level() < Level::Info { + fn enabled(&self, criteria: &FilterCriteria, _: &[Box]) -> FilterResult { + if criteria.level() < Level::Info { FilterResult::Accept } else { FilterResult::Reject From da8f85f80b3020f0cc3451d2bad8cc67a04c4be2 Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 3 Nov 2025 13:59:41 +0800 Subject: [PATCH 2/4] rework str Signed-off-by: tison --- core/src/kv.rs | 33 ++---- core/src/record.rs | 95 +++++++---------- core/src/str.rs | 249 +++++++++++++++------------------------------ 3 files changed, 130 insertions(+), 247 deletions(-) diff --git a/core/src/kv.rs b/core/src/kv.rs index 6d4ff34..4a79685 100644 --- a/core/src/kv.rs +++ b/core/src/kv.rs @@ -14,20 +14,17 @@ //! Key-value pairs in a log record or a diagnostic context. -// This file is derived from https://github.com/SpriteOvO/spdlog-rs/blob/788bda33/spdlog/src/kv.rs - pub extern crate value_bag; use std::borrow::Cow; use std::fmt; use std::slice; -use std::sync::Arc; use value_bag::OwnedValueBag; use value_bag::ValueBag; use crate::Error; -use crate::str::Str; +use crate::str::{OwnedStr, RefStr}; /// A visitor to walk through key-value pairs. pub trait Visitor { @@ -40,20 +37,18 @@ pub type Value<'a> = ValueBag<'a>; /// A key in a key-value pair. #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct Key<'a>(Str<'a>); +pub struct Key<'a>(RefStr<'a>); impl Key<'static> { /// Create a new key from a static `&str`. pub const fn new(k: &'static str) -> Key<'static> { - Key(Str::new(k)) + Key(RefStr::Static(k)) } +} - /// Create a new key from a shared value. - /// - /// Cloning the key will involve cloning the `Arc`, which may be cheaper than cloning the - /// value itself. - pub fn new_shared(key: impl Into>) -> Self { - Key(Str::new_shared(key)) +impl fmt::Display for Key<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0, f) } } @@ -62,17 +57,17 @@ impl<'a> Key<'a> { /// /// The [`Key::new`] method should be preferred where possible. pub const fn new_ref(k: &'a str) -> Key<'a> { - Key(Str::new_ref(k)) + Key(RefStr::Borrowed(k)) } /// Convert to an owned key. pub fn to_owned(&self) -> KeyOwned { - KeyOwned(self.0.to_shared()) + KeyOwned(self.0.to_owned()) } /// Convert to a `Cow` str. pub fn to_cow(&self) -> Cow<'static, str> { - self.0.to_cow() + self.0.to_cow_static() } /// Get the key string. @@ -81,18 +76,12 @@ impl<'a> Key<'a> { } } -impl fmt::Display for Key<'_> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(&self.0, f) - } -} - /// An owned value in a key-value pair. pub type ValueOwned = OwnedValueBag; /// An owned key in a key-value pair. #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct KeyOwned(Str<'static>); +pub struct KeyOwned(OwnedStr); impl KeyOwned { /// Create a `Key` ref. diff --git a/core/src/record.rs b/core/src/record.rs index b7a6f25..94a9681 100644 --- a/core/src/record.rs +++ b/core/src/record.rs @@ -22,39 +22,7 @@ use std::time::SystemTime; use crate::Error; use crate::kv; use crate::kv::KeyValues; -use crate::str::Str; - -// This struct is preferred over `Str` because we need to return a &'a str -// when holding only a reference to the str ref. But `Str::get` return a &str -// that lives as long as the `Str` itself, which is not necessarily 'a. -#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] -enum MaybeStaticStr<'a> { - Str(&'a str), - Static(&'static str), -} - -impl<'a> MaybeStaticStr<'a> { - fn get(&self) -> &'a str { - match *self { - MaybeStaticStr::Str(s) => s, - MaybeStaticStr::Static(s) => s, - } - } - - fn get_static(&self) -> Option<&'static str> { - match *self { - MaybeStaticStr::Str(_) => None, - MaybeStaticStr::Static(s) => Some(s), - } - } - - fn into_str(self) -> Str<'static> { - match self { - MaybeStaticStr::Str(s) => Str::new_shared(s), - MaybeStaticStr::Static(s) => Str::new(s), - } - } -} +use crate::str::{OwnedStr, RefStr}; /// The payload of a log message. #[derive(Clone, Debug)] @@ -64,13 +32,13 @@ pub struct Record<'a> { // the metadata level: Level, - target: &'a str, - module_path: Option>, - file: Option>, + target: RefStr<'a>, + module_path: Option>, + file: Option>, line: Option, // the payload - payload: Str<'static>, + payload: OwnedStr, // structural logging kvs: KeyValues<'a>, @@ -89,7 +57,12 @@ impl<'a> Record<'a> { /// The name of the target of the directive. pub fn target(&self) -> &'a str { - self.target + self.target.get() + } + + /// The name of the target of the directive, if it is a `'static` str. + pub fn target_static(&self) -> Option<&'a str> { + self.target.get_static() } /// The module path of the message. @@ -148,9 +121,9 @@ impl<'a> Record<'a> { RecordOwned { now: self.now, level: self.level, - target: Str::new_shared(self.target), - module_path: self.module_path.map(MaybeStaticStr::into_str), - file: self.file.map(MaybeStaticStr::into_str), + target: self.target.to_owned(), + module_path: self.module_path.as_ref().map(RefStr::to_owned), + file: self.file.as_ref().map(RefStr::to_owned), line: self.line, payload: self.payload.clone(), kvs: self @@ -195,11 +168,11 @@ impl Default for RecordBuilder<'_> { record: Record { now: SystemTime::now(), level: Level::Info, - target: "", + target: RefStr::Static(""), module_path: None, file: None, line: None, - payload: Default::default(), + payload: OwnedStr::Static(""), kvs: Default::default(), }, } @@ -210,8 +183,8 @@ impl<'a> RecordBuilder<'a> { /// Set [`payload`](Record::payload). pub fn payload(mut self, payload: impl Into>) -> Self { self.record.payload = match payload.into() { - Cow::Borrowed(s) => Str::new(s), - Cow::Owned(s) => Str::new_shared(s), + Cow::Borrowed(s) => OwnedStr::Static(s), + Cow::Owned(s) => OwnedStr::Owned(s.into_boxed_str()), }; self } @@ -224,31 +197,37 @@ impl<'a> RecordBuilder<'a> { /// Set [`target`](Record::target). pub fn target(mut self, target: &'a str) -> Self { - self.record.target = target; + self.record.target = RefStr::Borrowed(target); + self + } + + /// Set [`target`](Record::target) to a `'static` string. + pub fn target_static(mut self, target: &'static str) -> Self { + self.record.target = RefStr::Static(target); self } /// Set [`module_path`](Record::module_path). pub fn module_path(mut self, path: Option<&'a str>) -> Self { - self.record.module_path = path.map(MaybeStaticStr::Str); + self.record.module_path = path.map(RefStr::Borrowed); self } /// Set [`module_path`](Record::module_path) to a `'static` string. pub fn module_path_static(mut self, path: &'static str) -> Self { - self.record.module_path = Some(MaybeStaticStr::Static(path)); + self.record.module_path = Some(RefStr::Static(path)); self } /// Set [`file`](Record::file). pub fn file(mut self, file: Option<&'a str>) -> Self { - self.record.file = file.map(MaybeStaticStr::Str); + self.record.file = file.map(RefStr::Borrowed); self } /// Set [`file`](Record::file) to a `'static` string. pub fn file_static(mut self, file: &'static str) -> Self { - self.record.file = Some(MaybeStaticStr::Static(file)); + self.record.file = Some(RefStr::Static(file)); self } @@ -315,7 +294,7 @@ impl Default for FilterCriteriaBuilder<'_> { FilterCriteriaBuilder { metadata: FilterCriteria { level: Level::Info, - target: Default::default(), + target: "", }, } } @@ -348,13 +327,13 @@ pub struct RecordOwned { // the metadata level: Level, - target: Str<'static>, - module_path: Option>, - file: Option>, + target: OwnedStr, + module_path: Option, + file: Option, line: Option, // the payload - payload: Str<'static>, + payload: OwnedStr, // structural logging kvs: Vec<(kv::KeyOwned, kv::ValueOwned)>, @@ -366,9 +345,9 @@ impl RecordOwned { Record { now: self.now, level: self.level, - target: self.target.get(), - module_path: self.module_path.as_deref().map(MaybeStaticStr::Str), - file: self.file.as_deref().map(MaybeStaticStr::Str), + target: self.target.by_ref(), + module_path: self.module_path.as_ref().map(OwnedStr::by_ref), + file: self.file.as_ref().map(OwnedStr::by_ref), line: self.line, payload: self.payload.clone(), kvs: KeyValues::from(self.kvs.as_slice()), diff --git a/core/src/str.rs b/core/src/str.rs index aaac744..7e2c4f7 100644 --- a/core/src/str.rs +++ b/core/src/str.rs @@ -12,238 +12,153 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! The [`Str`] type. -//! -//! This module implements a string type that combines `Cow<'static, str>` with `Cow<'a, str>`. A -//! [`Str`] can hold borrowed, static, or shared data. Internally, it's more efficient than a -//! [`Cow`] to access because it doesn't need to hop through enum variants. - -// This file is derived from https://github.com/emit-rs/emit/blob/097f5254/core/src/str.rs - -use std::borrow::Borrow; use std::borrow::Cow; use std::cmp::Ordering; use std::fmt; -use std::hash; -use std::marker::PhantomData; +use std::hash::Hash; use std::sync::Arc; -/// A string value. -pub struct Str<'k> { - // This type is an optimized `Cow` - // It avoids the cost of matching the variant to get the inner value - value: *const str, - owner: StrOwner, - marker: PhantomData<&'k str>, -} - -// SAFETY: `Str` synchronizes through `Arc` when ownership is shared -unsafe impl<'k> Send for Str<'k> {} -// SAFETY: `Str` does not use interior mutability -unsafe impl<'k> Sync for Str<'k> {} - -impl<'k> Default for Str<'k> { - fn default() -> Self { - Str::new(Default::default()) - } -} - -enum StrOwner { - None, +#[derive(Clone, Copy)] +pub enum RefStr<'a> { + Borrowed(&'a str), Static(&'static str), - Shared(Arc), } -impl<'k> fmt::Debug for Str<'k> { +impl fmt::Debug for RefStr<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Debug::fmt(self.get(), f) } } -impl<'k> fmt::Display for Str<'k> { +impl fmt::Display for RefStr<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(self.get(), f) } } -impl<'k> Clone for Str<'k> { - fn clone(&self) -> Self { - match self.owner { - StrOwner::Shared(ref value) => Str::new_shared(value.clone()), - StrOwner::Static(owner) => Str { - value: self.value, - owner: StrOwner::Static(owner), - marker: PhantomData, - }, - StrOwner::None => Str { - value: self.value, - owner: StrOwner::None, - marker: PhantomData, - }, +impl<'a> RefStr<'a> { + pub const fn get(&self) -> &'a str { + match self { + RefStr::Borrowed(s) => s, + RefStr::Static(s) => s, } } -} -impl Str<'static> { - /// Create a new string from a value borrowed for `'static`. - pub const fn new(k: &'static str) -> Self { - Str { - value: k as *const str, - owner: StrOwner::Static(k), - marker: PhantomData, + pub const fn get_static(&self) -> Option<&'static str> { + match self { + RefStr::Borrowed(_) => None, + RefStr::Static(s) => Some(s), } } - /// Create a string from a shared value. - /// - /// Cloning the string will involve cloning the `Arc`, which may be cheaper than cloning the - /// value itself. - pub fn new_shared(key: impl Into>) -> Self { - let value = key.into(); - - Str { - value: &*value as *const str, - owner: StrOwner::Shared(value), - marker: PhantomData, + pub fn to_cow_static(&self) -> Cow<'static, str> { + match self { + RefStr::Borrowed(s) => Cow::Owned(ToOwned::to_owned(*s)), + RefStr::Static(s) => Cow::Borrowed(s), } } -} -impl<'k> Str<'k> { - /// Create a new string from a value borrowed for `'k`. - /// - /// The [`Str::new`] method should be preferred where possible. - pub const fn new_ref(k: &'k str) -> Str<'k> { - Str { - value: k as *const str, - owner: StrOwner::None, - marker: PhantomData, + pub fn to_owned(&self) -> OwnedStr { + match self { + RefStr::Borrowed(s) => OwnedStr::Owned(Box::from(*s)), + RefStr::Static(s) => OwnedStr::Static(s), } } +} - /// Get a new [`Str`], borrowing data from this one. - pub const fn by_ref(&self) -> Str<'_> { - Str { - value: self.value, - owner: match self.owner { - StrOwner::Static(owner) => StrOwner::Static(owner), - StrOwner::None | StrOwner::Shared(_) => StrOwner::None, - }, - marker: PhantomData, - } +impl PartialEq for RefStr<'_> { + fn eq(&self, other: &Self) -> bool { + PartialEq::eq(self.get(), other.get()) } +} - /// Get a reference to the underlying value. - pub const fn get(&self) -> &str { - // NOTE: It's important here that the lifetime returned is not `'k` - // If it was it would be possible to return a `&'static str` from - // an owned value - // SAFETY: `self.value` is guaranteed to outlive the borrow of `self` - unsafe { &(*self.value) } - } +impl Eq for RefStr<'_> {} - /// Try to get a reference to the underlying static value. - /// - /// If the string was created from [`Str::new`] and contains a `'static` value then this method - /// will return `Some`. Otherwise, this method will return `None`. - pub const fn get_static(&self) -> Option<&'static str> { - if let StrOwner::Static(owner) = self.owner { - Some(owner) - } else { - None - } - } - - /// Get a new string, taking an owned copy of the data in this one. - /// - /// If the string contains a `'static` or `Arc` value then this method is cheap. In other cases - /// the underlying value will be passed through [`Str::new_shared`]. - pub fn to_shared(&self) -> Str<'static> { - match self.owner { - StrOwner::Static(owner) => Str::new(owner), - StrOwner::Shared(ref owner) => Str::new_shared(owner.clone()), - StrOwner::None => Str::new_shared(self.get()), - } +impl PartialOrd for RefStr<'_> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) } +} - /// Get the underlying value as a potentially owned string. - /// - /// If the string contains a `'static` value then this method will return `Cow::Borrowed`. - /// Otherwise, it will return `Cow::Owned`. - pub fn to_cow(&self) -> Cow<'static, str> { - match self.owner { - StrOwner::Static(key) => Cow::Borrowed(key), - StrOwner::None | StrOwner::Shared(_) => Cow::Owned(self.get().to_owned()), - } +impl Ord for RefStr<'_> { + fn cmp(&self, other: &Self) -> Ordering { + Ord::cmp(self.get(), other.get()) } } -impl<'a> hash::Hash for Str<'a> { - fn hash(&self, state: &mut H) { - self.get().hash(state) +impl Hash for RefStr<'_> { + fn hash(&self, state: &mut H) { + Hash::hash(self.get(), state) } } -impl std::ops::Deref for Str<'_> { - type Target = str; +#[derive(Clone)] +pub enum OwnedStr { + Owned(Box), + Static(&'static str), + Shared(Arc), +} - fn deref(&self) -> &Self::Target { - self.get() +impl fmt::Debug for OwnedStr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(self.get(), f) } } -impl<'a, 'b> PartialEq> for Str<'a> { - fn eq(&self, other: &Str<'b>) -> bool { - self.get() == other.get() +impl fmt::Display for OwnedStr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self.get(), f) } } -impl<'a> Eq for Str<'a> {} -impl<'a> PartialEq for Str<'a> { - fn eq(&self, other: &str) -> bool { - self.get() == other +impl OwnedStr { + pub fn get(&self) -> &str { + match self { + OwnedStr::Owned(s) => s, + OwnedStr::Static(s) => s, + OwnedStr::Shared(s) => s, + } } -} -impl<'a> PartialEq> for str { - fn eq(&self, other: &Str<'a>) -> bool { - self == other.get() + pub fn get_static(&self) -> Option<&'static str> { + match self { + OwnedStr::Owned(_) | OwnedStr::Shared(_) => None, + OwnedStr::Static(s) => Some(s), + } } -} -impl<'a, 'b> PartialEq<&'b str> for Str<'a> { - fn eq(&self, other: &&'b str) -> bool { - self.get() == *other + pub fn by_ref(&self) -> RefStr<'_> { + match self { + OwnedStr::Owned(s) => RefStr::Borrowed(s), + OwnedStr::Static(s) => RefStr::Static(s), + OwnedStr::Shared(s) => RefStr::Borrowed(s), + } } } -impl<'b> PartialEq> for &str { - fn eq(&self, other: &Str<'b>) -> bool { - *self == other.get() +impl PartialEq for OwnedStr { + fn eq(&self, other: &Self) -> bool { + PartialEq::eq(self.get(), other.get()) } } -impl<'a, 'b> PartialOrd> for Str<'a> { - fn partial_cmp(&self, other: &Str<'b>) -> Option { - self.get().partial_cmp(other.get()) - } -} +impl Eq for OwnedStr {} -impl<'a> Ord for Str<'a> { - fn cmp(&self, other: &Self) -> Ordering { - self.get().cmp(other.get()) +impl PartialOrd for OwnedStr { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) } } -impl<'k> Borrow for Str<'k> { - fn borrow(&self) -> &str { - self.get() +impl Ord for OwnedStr { + fn cmp(&self, other: &Self) -> Ordering { + Ord::cmp(self.get(), other.get()) } } -impl<'k> AsRef for Str<'k> { - fn as_ref(&self) -> &str { - self.get() +impl Hash for OwnedStr { + fn hash(&self, state: &mut H) { + Hash::hash(self.get(), state) } } From a64cc80c39ef4d130f1cb3a1767e1ac519afaa7a Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 3 Nov 2025 14:06:50 +0800 Subject: [PATCH 3/4] fine tune str Signed-off-by: tison --- core/src/kv.rs | 4 ++-- core/src/record.rs | 6 +++--- core/src/str.rs | 15 +++++---------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/core/src/kv.rs b/core/src/kv.rs index 4a79685..58d165e 100644 --- a/core/src/kv.rs +++ b/core/src/kv.rs @@ -62,12 +62,12 @@ impl<'a> Key<'a> { /// Convert to an owned key. pub fn to_owned(&self) -> KeyOwned { - KeyOwned(self.0.to_owned()) + KeyOwned(self.0.into_owned()) } /// Convert to a `Cow` str. pub fn to_cow(&self) -> Cow<'static, str> { - self.0.to_cow_static() + self.0.into_cow_static() } /// Get the key string. diff --git a/core/src/record.rs b/core/src/record.rs index 94a9681..bd0117e 100644 --- a/core/src/record.rs +++ b/core/src/record.rs @@ -121,9 +121,9 @@ impl<'a> Record<'a> { RecordOwned { now: self.now, level: self.level, - target: self.target.to_owned(), - module_path: self.module_path.as_ref().map(RefStr::to_owned), - file: self.file.as_ref().map(RefStr::to_owned), + target: self.target.into_owned(), + module_path: self.module_path.map(RefStr::into_owned), + file: self.file.map(RefStr::into_owned), line: self.line, payload: self.payload.clone(), kvs: self diff --git a/core/src/str.rs b/core/src/str.rs index 7e2c4f7..6ada0e8 100644 --- a/core/src/str.rs +++ b/core/src/str.rs @@ -16,7 +16,6 @@ use std::borrow::Cow; use std::cmp::Ordering; use std::fmt; use std::hash::Hash; -use std::sync::Arc; #[derive(Clone, Copy)] pub enum RefStr<'a> { @@ -51,16 +50,16 @@ impl<'a> RefStr<'a> { } } - pub fn to_cow_static(&self) -> Cow<'static, str> { + pub fn into_cow_static(self) -> Cow<'static, str> { match self { - RefStr::Borrowed(s) => Cow::Owned(ToOwned::to_owned(*s)), + RefStr::Borrowed(s) => Cow::Owned(ToOwned::to_owned(s)), RefStr::Static(s) => Cow::Borrowed(s), } } - pub fn to_owned(&self) -> OwnedStr { + pub fn into_owned(self) -> OwnedStr { match self { - RefStr::Borrowed(s) => OwnedStr::Owned(Box::from(*s)), + RefStr::Borrowed(s) => OwnedStr::Owned(Box::from(s)), RefStr::Static(s) => OwnedStr::Static(s), } } @@ -96,7 +95,6 @@ impl Hash for RefStr<'_> { pub enum OwnedStr { Owned(Box), Static(&'static str), - Shared(Arc), } impl fmt::Debug for OwnedStr { @@ -111,19 +109,17 @@ impl fmt::Display for OwnedStr { } } - impl OwnedStr { pub fn get(&self) -> &str { match self { OwnedStr::Owned(s) => s, OwnedStr::Static(s) => s, - OwnedStr::Shared(s) => s, } } pub fn get_static(&self) -> Option<&'static str> { match self { - OwnedStr::Owned(_) | OwnedStr::Shared(_) => None, + OwnedStr::Owned(_) => None, OwnedStr::Static(s) => Some(s), } } @@ -132,7 +128,6 @@ impl OwnedStr { match self { OwnedStr::Owned(s) => RefStr::Borrowed(s), OwnedStr::Static(s) => RefStr::Static(s), - OwnedStr::Shared(s) => RefStr::Borrowed(s), } } } From 70e12079ed240ac630be1d910b2b51ec6466c8ff Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 3 Nov 2025 14:07:39 +0800 Subject: [PATCH 4/4] fmt Signed-off-by: tison --- core/src/kv.rs | 3 ++- core/src/record.rs | 55 ++++++++++++++++++++++++---------------------- 2 files changed, 31 insertions(+), 27 deletions(-) diff --git a/core/src/kv.rs b/core/src/kv.rs index 58d165e..98f5fee 100644 --- a/core/src/kv.rs +++ b/core/src/kv.rs @@ -24,7 +24,8 @@ use value_bag::OwnedValueBag; use value_bag::ValueBag; use crate::Error; -use crate::str::{OwnedStr, RefStr}; +use crate::str::OwnedStr; +use crate::str::RefStr; /// A visitor to walk through key-value pairs. pub trait Visitor { diff --git a/core/src/record.rs b/core/src/record.rs index bd0117e..f1f1198 100644 --- a/core/src/record.rs +++ b/core/src/record.rs @@ -22,7 +22,8 @@ use std::time::SystemTime; use crate::Error; use crate::kv; use crate::kv::KeyValues; -use crate::str::{OwnedStr, RefStr}; +use crate::str::OwnedStr; +use crate::str::RefStr; /// The payload of a log message. #[derive(Clone, Debug)] @@ -570,38 +571,40 @@ impl FromStr for Level { #[cfg(test)] mod tests { + use super::*; + #[test] fn round_trip_level() { let levels = [ - super::Level::Trace, - super::Level::Trace2, - super::Level::Trace3, - super::Level::Trace4, - super::Level::Debug, - super::Level::Debug2, - super::Level::Debug3, - super::Level::Debug4, - super::Level::Info, - super::Level::Info2, - super::Level::Info3, - super::Level::Info4, - super::Level::Warn, - super::Level::Warn2, - super::Level::Warn3, - super::Level::Warn4, - super::Level::Error, - super::Level::Error2, - super::Level::Error3, - super::Level::Error4, - super::Level::Fatal, - super::Level::Fatal2, - super::Level::Fatal3, - super::Level::Fatal4, + Level::Trace, + Level::Trace2, + Level::Trace3, + Level::Trace4, + Level::Debug, + Level::Debug2, + Level::Debug3, + Level::Debug4, + Level::Info, + Level::Info2, + Level::Info3, + Level::Info4, + Level::Warn, + Level::Warn2, + Level::Warn3, + Level::Warn4, + Level::Error, + Level::Error2, + Level::Error3, + Level::Error4, + Level::Fatal, + Level::Fatal2, + Level::Fatal3, + Level::Fatal4, ]; for &level in &levels { let s = level.name(); - let parsed = s.parse::().unwrap(); + let parsed = s.parse::().unwrap(); assert_eq!(level, parsed); } }