diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46d1816..520ab53 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,10 +5,6 @@ on: branches: - main pull_request: - branches: - - main - schedule: - - cron: "00 01 * * *" env: RUST_BACKTRACE: short @@ -42,8 +38,8 @@ jobs: - nightly - arm32 - arm64 - - mips32 - - mips64 + - ppc32 + - ppc64 include: - build: linux os: ubuntu-latest @@ -63,14 +59,9 @@ jobs: - build: win32-gnu os: windows-2019 rust: stable-i686-gnu - # TODO: re-enable this I guess. I get inscrutable errors like - # `error reading from the zlib stream; class=Zlib (5)` in CI - # that don't repro locally, and I'm tired of getting an email - # about this every day. - # - # - build: msrv - # os: ubuntu-latest - # rust: "1.43.0" + - build: msrv + os: ubuntu-latest + rust: "1.57.0" - build: beta os: ubuntu-latest rust: beta @@ -90,16 +81,16 @@ jobs: os: ubuntu-latest rust: stable target: aarch64-unknown-linux-gnu - # Mips is big endian. Nothing currently in here cares... but will if I + # PPC is big endian. Nothing currently in here cares... but will if I # ever get around to that `key` stuff. - - build: mips32 + - build: ppc32 os: ubuntu-latest rust: stable - target: mips-unknown-linux-gnu - - build: mips64 + target: powerpc-unknown-linux-gnu + - build: ppc64 os: ubuntu-latest rust: stable - target: mips64-unknown-linux-gnuabi64 + target: powerpc64-unknown-linux-gnu # Requested by a user not sure if it adds anything we aren't already # testing but it's easy enough so *shrug*. - build: riscv @@ -108,12 +99,10 @@ jobs: target: riscv64gc-unknown-linux-gnu steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 + - uses: actions/checkout@v4 + - uses: hecrj/setup-rust-action@v2 with: - toolchain: ${{ matrix.rust }} - profile: minimal - override: true + rust-version: ${{ matrix.rust }} - uses: taiki-e/install-action@cross if: matrix.target != '' @@ -145,76 +134,46 @@ jobs: env: RUSTFLAGS: --cfg loom -Dwarnings steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - profile: minimal - override: true + - uses: actions/checkout@v4 + - uses: hecrj/setup-rust-action@v2 - run: cargo test --all-features --lib - run: cargo test --no-default-features --lib miri: name: Miri runs-on: ubuntu-latest - env: - MIRIFLAGS: -Zmiri-tag-raw-pointers steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 + - uses: actions/checkout@v4 + - uses: hecrj/setup-rust-action@v2 with: - toolchain: nightly + rust-version: nightly components: miri, rust-src - override: true - run: cargo miri test --all-features - run: cargo miri test --features="std serde substr" - run: cargo miri test - cargo-clippy: + cargo-check: name: Lint runs-on: ubuntu-latest env: RUSTFLAGS: -Dwarnings steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - components: clippy - minimal: true - override: true - - run: cargo clippy --workspace --all-targets --verbose - - run: cargo clippy --workspace --all-targets --verbose --all-features - - run: cargo clippy --workspace --all-targets --verbose --features="serde std substr" - - run: cargo clippy --workspace --all-targets --verbose --no-default-features + - uses: actions/checkout@v4 + - uses: hecrj/setup-rust-action@v2 + - run: cargo check --workspace --all-targets --verbose + - run: cargo check --workspace --all-targets --verbose --all-features + - run: cargo check --workspace --all-targets --verbose --features="serde std substr" + - run: cargo check --workspace --all-targets --verbose --no-default-features # Ensure patch is formatted. fmt: name: Format runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - components: rustfmt - minimal: true - override: true + - uses: actions/checkout@v4 + - uses: hecrj/setup-rust-action@v2 - run: cargo fmt --all -- --check - # Check doc reference links are all valid. - doc: - name: Doc check - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - minimal: true - override: true - - run: cargo doc --features="serde std substr" - sanitizers: name: Test sanitizer ${{ matrix.sanitizer }} runs-on: ubuntu-latest @@ -233,7 +192,7 @@ jobs: extra_rustflags: "-Zsanitizer-memory-track-origins" steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: actions-rs/toolchain@v1 with: profile: minimal @@ -251,17 +210,15 @@ jobs: cargo -Zbuild-std test --target=x86_64-unknown-linux-gnu --all-features cargo -Zbuild-std test --target=x86_64-unknown-linux-gnu --no-default-features - codecov: - name: Generate code coverage + codecov-tarpaulin: + name: coverage runs-on: ubuntu-latest + container: + image: xd009642/tarpaulin:develop-nightly + options: --security-opt seccomp=unconfined steps: - - uses: actions/checkout@v3 - - name: Install stable toolchain - uses: actions-rs/toolchain@v1 + - uses: actions/checkout@v4 + - run: cargo tarpaulin --verbose --doc --all-features --all-targets --engine llvm --out xml + - uses: codecov/codecov-action@v4 with: - toolchain: stable - override: true - - name: Run cargo-tarpaulin - uses: actions-rs/tarpaulin@v0.1 - - name: Upload to codecov.io - uses: codecov/codecov-action@v1 + token: ${{ secrets.CODECOV_TOKEN }} diff --git a/Cargo.toml b/Cargo.toml index df45569..73994a2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,8 +1,9 @@ [package] name = "arcstr" version = "1.1.5" +rust-version = "1.57.0" authors = ["Thom Chiovoloni "] -edition = "2018" +edition = "2021" description = "A better reference-counted string type, with zero-cost (allocation-free) support for string literals, and reference counted substrings." license = "Apache-2.0 OR MIT OR Zlib" readme = "README.md" @@ -29,10 +30,9 @@ serde = { version = "1", default-features = false, optional = true } [dev-dependencies] serde_test = { version = "1", default-features = false } -memoffset = "0.6" [target.'cfg(loom)'.dev-dependencies] -loom = "0.5.6" +loom = "0.7.1" [package.metadata.docs.rs] features = ["std", "substr"] diff --git a/README.md b/README.md index 2c4d44d..0e84a8e 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ [![codecov](https://codecov.io/gh/thomcc/arcstr/branch/main/graph/badge.svg)](https://codecov.io/gh/thomcc/arcstr) [![Docs](https://docs.rs/arcstr/badge.svg)](https://docs.rs/arcstr) [![Latest Version](https://img.shields.io/crates/v/arcstr.svg)](https://crates.io/crates/arcstr) -![Minimum Rust Version](https://img.shields.io/badge/MSRV%201.43-blue.svg) +![Minimum Rust Version](https://img.shields.io/badge/MSRV%201.57-blue.svg) This crate defines `ArcStr`, a reference counted string type. It's essentially trying to be a better `Arc` or `Arc`, at least for most use cases. diff --git a/src/arc_str.rs b/src/arc_str.rs index 1e682dd..d6a248d 100644 --- a/src/arc_str.rs +++ b/src/arc_str.rs @@ -160,7 +160,7 @@ impl ArcStr { /// ``` #[inline] pub fn try_alloc(copy_from: &str) -> Option { - if let Ok(inner) = ThinInner::try_allocate(copy_from) { + if let Ok(inner) = ThinInner::try_allocate(copy_from, false) { Some(Self(inner)) } else { None @@ -196,12 +196,12 @@ impl ArcStr { /// ``` #[inline] pub fn len(&self) -> usize { - self.get_inner_len_flags().len() + self.get_inner_len_flag().uint_part() } #[inline] - fn get_inner_len_flags(&self) -> LenFlags { - unsafe { ThinInner::get_len_flags(self.0.as_ptr()) } + fn get_inner_len_flag(&self) -> PackedFlagUint { + unsafe { ThinInner::get_len_flag(self.0.as_ptr()) } } /// Returns true if this `ArcStr` is empty. @@ -252,10 +252,8 @@ impl ArcStr { let len = self.len(); let p = self.0.as_ptr(); unsafe { - let data = (p as *const u8).add(OFFSET_DATA); - - debug_assert_eq!(&(*p).data as *const [u8; 0] as usize, data as usize); - + let data = p.cast::().add(OFFSET_DATA); + debug_assert_eq!(core::ptr::addr_of!((*p).data).cast::(), data); core::slice::from_raw_parts(data, len) } } @@ -382,13 +380,92 @@ impl ArcStr { /// ``` #[inline] pub fn strong_count(this: &Self) -> Option { - if Self::is_static(this) { + let cf = Self::load_count_flag(this, Ordering::Acquire)?; + if cf.flag_part() { + None + } else { + Some(cf.uint_part()) + } + } + + /// Safety: Unsafe to use `this` is stored in static memory (check + /// `Self::has_static_lenflag`) + #[inline] + unsafe fn load_count_flag_raw(this: &Self, ord_if_needed: Ordering) -> PackedFlagUint { + PackedFlagUint::from_encoded((*this.0.as_ptr()).count_flag.load(ord_if_needed)) + } + + #[inline] + fn load_count_flag(this: &Self, ord_if_needed: Ordering) -> Option { + if Self::has_static_lenflag(this) { None } else { - Some(unsafe { (*this.0.as_ptr()).strong.load(Ordering::SeqCst) }) + let count_and_flag = PackedFlagUint::from_encoded(unsafe { + (*this.0.as_ptr()).count_flag.load(ord_if_needed) + }); + Some(count_and_flag) + } + } + + /// Convert the `ArcStr` into a `'static` `ArcStr`, even if it was + /// originally created from runtime values. + /// + /// This is useful + /// + /// If the `ArcStr` is already static, then + #[inline] + pub fn leak(this: &Self) -> &'static str { + if Self::has_static_lenflag(this) { + return unsafe { Self::to_static_unchecked(this) }; + } + let is_static_count = unsafe { + // Not sure about ordering, maybe relaxed would be fine. + Self::load_count_flag_raw(this, Ordering::Acquire) + }; + if is_static_count.flag_part() { + return unsafe { Self::to_static_unchecked(this) }; + } + unsafe { Self::become_static(this, is_static_count.uint_part() == 1) }; + debug_assert!(Self::is_static(this)); + unsafe { Self::to_static_unchecked(this) } + } + + unsafe fn become_static(this: &Self, is_unique: bool) { + if is_unique { + core::ptr::addr_of_mut!((*this.0.as_ptr()).count_flag).write(AtomicUsize::new( + PackedFlagUint::new_raw(true, 1).encoded_value(), + )); + let lenp = core::ptr::addr_of_mut!((*this.0.as_ptr()).len_flag); + debug_assert!(!lenp.read().flag_part()); + lenp.write(lenp.read().with_flag(true)); + } else { + let flag_bit = PackedFlagUint::new_raw(true, 0).encoded_value(); + let atomic_count_flag = &*core::ptr::addr_of!((*this.0.as_ptr()).count_flag); + atomic_count_flag.fetch_or(flag_bit, Ordering::Release); + } + } + + #[inline] + unsafe fn to_static_unchecked(this: &Self) -> &'static str { + &*Self::str_ptr(this) + } + + #[inline] + fn bytes_ptr(this: &Self) -> *const [u8] { + let len = this.get_inner_len_flag().uint_part(); + unsafe { + let p: *const ThinInner = this.0.as_ptr(); + let data = p.cast::().add(OFFSET_DATA); + debug_assert_eq!(core::ptr::addr_of!((*p).data).cast::(), data,); + core::ptr::slice_from_raw_parts(data, len) } } + #[inline] + fn str_ptr(this: &Self) -> *const str { + Self::bytes_ptr(this) as *const str + } + /// Returns true if `this` is a "static" ArcStr. For example, if it was /// created from a call to [`arcstr::literal!`][crate::literal]), /// returned by `ArcStr::new`, etc. @@ -416,7 +493,32 @@ impl ArcStr { /// ``` #[inline] pub fn is_static(this: &Self) -> bool { - this.get_inner_len_flags().is_static() + // We align this to 16 bytes and keep the `is_static` flags in the same + // place. In theory this means that if `cfg(target_feature = "avx")` + // (where aligned 16byte loads are atomic), the compiler *could* + // implement this function using the equivalent of: + // ``` + // let vec = _mm_load_si128(self.0.as_ptr().cast()); + // let mask = _mm_movemask_pd(_mm_srli_epi64(vac, 63)); + // mask != 0 + // ``` + // and that's all; one load, no branching. (I don't think it *does*, but + // I haven't checked so I'll be optimistic and keep the `#[repr(align)]` + // -- hey, maybe the CPU can peephole-optimize it). + // + // That said, unless I did it in asm, *I* can't implement it that way, + // since Rust's semantics don't allow me to make that change + // optimization on my own (that load isn't considered atomic, for + // example). + this.get_inner_len_flag().flag_part() + || unsafe { Self::load_count_flag_raw(this, Ordering::Relaxed).flag_part() } + } + + /// This is true for any `ArcStr` that has been static from the time when it + /// was created. It's cheaper than `has_static_rcflag`. + #[inline] + fn has_static_lenflag(this: &Self) -> bool { + this.get_inner_len_flag().flag_part() } /// Returns true if `this` is a "static"/`"literal"` ArcStr. For example, if @@ -669,7 +771,7 @@ impl ArcStr { // Calculate the capacity for the allocated string let capacity = source.len().checked_mul(n)?; - let inner = ThinInner::try_allocate_uninit(capacity).ok()?; + let inner = ThinInner::try_allocate_uninit(capacity, false).ok()?; unsafe { let mut data_ptr = ThinInner::data_ptr(inner); @@ -739,15 +841,29 @@ impl Clone for ArcStr { // > the object. // // See: https://doc.rust-lang.org/src/alloc/sync.rs.html#1073 - let n = unsafe { (*self.0.as_ptr()).strong.fetch_add(1, Ordering::Relaxed) }; - // Protect against aggressive leaking of Arcs causing us to overflow `strong`. - if n > (isize::MAX as usize) { - abort(); + let n: PackedFlagUint = PackedFlagUint::from_encoded(unsafe { + let step = PackedFlagUint::FALSE_ONE.encoded_value(); + (*self.0.as_ptr()) + .count_flag + .fetch_add(step, Ordering::Relaxed) + }); + // Protect against aggressive leaking of Arcs causing us to + // overflow. Technically, we could probably transition it to static + // here, but I haven't thought it through. + if n.uint_part() > RC_MAX && !n.flag_part() { + let val = PackedFlagUint::new_raw(true, 0).encoded_value(); + unsafe { + (*self.0.as_ptr()) + .count_flag + .fetch_or(val, Ordering::Release) + }; + // abort(); } } Self(self.0) } } +const RC_MAX: usize = PackedFlagUint::UINT_PART_MAX / 2; impl Drop for ArcStr { #[inline] @@ -755,34 +871,16 @@ impl Drop for ArcStr { if Self::is_static(self) { return; } - let this = self.0.as_ptr(); unsafe { - if (*this).strong.fetch_sub(1, Ordering::Release) == 1 { - // `libstd` uses a full acquire fence here but notes that it's - // possibly overkill. `triomphe`/`servo_arc` some of firefox ref - // counting uses a load like this. - // - // These are morally equivalent for this case, the fence being a - // bit more obvious and the load having slightly better perf in - // some theoretical scenarios... but for our use case both seem - // unnecessary. - // - // The intention behind these is to synchronize with `Release` - // writes to `strong` that are happening on other threads. That - // is, after the load (or fence), writes (any write, but - // specifically writes to any part of `this` are what we care - // about) from other threads which happened before the latest - // `Release` write to strong will become visible on this thread. - // - // The reason this feels unnecessary is that our data is - // entirely immutable outside `(*this).strong`. There are no - // writes we could possibly be interested in. - // - // That said, I'll keep (the cheaper variant of) it for now for - // easier auditing and such... an because I'm not 100% sure that - // changing the ordering here wouldn't require changing it for - // the fetch_sub above, or the fetch_add in `clone`... - let _ = (*this).strong.load(Ordering::Acquire); + let this = self.0.as_ptr(); + let enc = PackedFlagUint::from_encoded( + (*this) + .count_flag + .fetch_sub(PackedFlagUint::FALSE_ONE.encoded_value(), Ordering::Release), + ); + // Note: `enc == PackedFlagUint::FALSE_ONE` + if enc == PackedFlagUint::FALSE_ONE { + let _ = (*this).count_flag.load(Ordering::Acquire); ThinInner::destroy_cold(this) } } @@ -799,7 +897,7 @@ impl Drop for ArcStr { // static ones to not have any interior mutability, so that `const`s can use // them, and so that they may be stored in read-only memory. // -// We do this by keeping a flag in `len_flags` flag to indicate which case we're +// We do this by keeping a flag in `len_flag` flag to indicate which case we're // in, and maintaining the invariant that if we're a `StaticArcStrInner` **we // may never access `.strong` in any way or produce a `&ThinInner` pointing to // our data**. @@ -814,25 +912,48 @@ impl Drop for ArcStr { // https://github.com/rust-lang/unsafe-code-guidelines/issues/246 #[repr(C, align(8))] struct ThinInner { - len_flags: LenFlags, - // kind of a misnomer since there are no weak refs rn. XXX ever? - strong: AtomicUsize, + // Both of these are `PackedFlagUint`s that store `is_static` as the flag. + // + // The reason it's not just stored in len is because an ArcStr may become + // static after creation (via `ArcStr::leak`) and we don't need to do an + // atomic load to access the length (and not only because it would mess with + // optimization). + // + // The reason it's not just stored in the count is because it may be UB to + // do atomic loads from read-only memory. This is also the reason it's not + // stored in a separate atomic, and why doing an atomic load to access the + // length wouldn't be acceptable even if compilers were really good. + len_flag: PackedFlagUint, + count_flag: AtomicUsize, data: [u8; 0], } const OFFSET_LENFLAGS: usize = 0; -const OFFSET_STRONGCOUNT: usize = size_of::(); -const OFFSET_DATA: usize = OFFSET_STRONGCOUNT + size_of::(); +const OFFSET_COUNTFLAGS: usize = size_of::(); +const OFFSET_DATA: usize = OFFSET_COUNTFLAGS + size_of::(); // Not public API, exists for macros. #[repr(C, align(8))] #[doc(hidden)] pub struct StaticArcStrInner { - pub len_flags: usize, - pub count: usize, + pub len_flag: usize, + pub count_flag: usize, pub data: Buf, } +impl StaticArcStrInner { + #[doc(hidden)] + pub const STATIC_COUNT_VALUE: usize = PackedFlagUint::new_raw(true, 1).encoded_value(); + #[doc(hidden)] + #[inline] + pub const fn encode_len(v: usize) -> Option { + match PackedFlagUint::new(true, v) { + Some(v) => Some(v.encoded_value()), + None => None, + } + } +} + const _: [(); size_of::>()] = [(); 2 * size_of::()]; const _: [(); align_of::>()] = [(); 8]; @@ -847,44 +968,72 @@ const _: [(); align_of::()] = [(); align_of::()]; const _: [(); align_of::()] = [(); size_of::()]; const _: [(); size_of::()] = [(); size_of::()]; -const _: [(); align_of::()] = [(); align_of::()]; -const _: [(); size_of::()] = [(); size_of::()]; +const _: [(); align_of::()] = [(); align_of::()]; +const _: [(); size_of::()] = [(); size_of::()]; -#[derive(Clone, Copy)] +#[derive(Clone, Copy, PartialEq, Eq)] #[repr(transparent)] -struct LenFlags(usize); +struct PackedFlagUint(usize); +impl PackedFlagUint { + const UINT_PART_MAX: usize = (1 << (usize::BITS - 1)) - 1; + /// Encodes `false` as the flag and `1` as the uint. Used for a few things, + /// such as the amount we `fetch_add` by for refcounting, and so on. + const FALSE_ONE: Self = Self::new_raw(false, 1); -impl LenFlags { #[inline] - const fn len(self) -> usize { + const fn new(flag_part: bool, uint_part: usize) -> Option { + if uint_part > Self::UINT_PART_MAX { + None + } else { + Some(Self::new_raw(flag_part, uint_part)) + } + } + + #[inline(always)] + const fn new_raw(flag_part: bool, uint_part: usize) -> Self { + Self(flag_part as usize | (uint_part << 1)) + } + + #[inline(always)] + const fn uint_part(self) -> usize { self.0 >> 1 } - #[inline] - const fn is_static(self) -> bool { - (self.0 & 1) == 0 + + #[inline(always)] + const fn flag_part(self) -> bool { + (self.0 & 1) != 0 } - #[inline] - fn from_len_static(l: usize, is_static: bool) -> Option { - l.checked_mul(2).map(|l| Self(l | (!is_static as usize))) + #[inline(always)] + const fn from_encoded(v: usize) -> Self { + Self(v) } - #[inline] - const fn from_len_static_raw(l: usize, is_static: bool) -> Self { - Self(l << 1 | (!is_static as usize)) + + #[inline(always)] + const fn encoded_value(self) -> usize { + self.0 + } + + #[inline(always)] + #[must_use] + const fn with_flag(self, v: bool) -> Self { + Self(v as usize | self.0) } } const EMPTY: ArcStr = literal!(""); impl ThinInner { - fn allocate(data: &str) -> NonNull { - match Self::try_allocate(data) { + #[inline] + fn allocate(data: &str, initially_static: bool) -> NonNull { + match Self::try_allocate(data, initially_static) { Ok(v) => v, Err(None) => alloc_overflow(), Err(Some(layout)) => alloc::alloc::handle_alloc_error(layout), } } + #[inline] fn data_ptr(this: NonNull) -> *mut u8 { unsafe { this.as_ptr().cast::().add(OFFSET_DATA) } } @@ -893,7 +1042,10 @@ impl ThinInner { /// /// Returns `Err(Some(layout))` if we failed to allocate that layout, and /// `Err(None)` for integer overflow when computing layout - fn try_allocate_uninit(capacity: usize) -> Result, Option> { + fn try_allocate_uninit( + capacity: usize, + initially_static: bool, + ) -> Result, Option> { const ALIGN: usize = align_of::(); debug_assert_ne!(capacity, 0); @@ -909,21 +1061,23 @@ impl ThinInner { } // we actually already checked this above... - debug_assert!(LenFlags::from_len_static(capacity, false).is_some()); + debug_assert!(PackedFlagUint::new(initially_static, capacity).is_some()); - let length_flags = LenFlags::from_len_static_raw(capacity, false); - debug_assert_eq!(length_flags.len(), capacity); - debug_assert!(!length_flags.is_static()); + let len_flag = PackedFlagUint::new_raw(initially_static, capacity); + debug_assert_eq!(len_flag.uint_part(), capacity); + debug_assert_eq!(len_flag.flag_part(), initially_static); unsafe { - core::ptr::write(&mut (*ptr).len_flags, length_flags); - core::ptr::write(&mut (*ptr).strong, AtomicUsize::new(1)); + core::ptr::addr_of_mut!((*ptr).len_flag).write(len_flag); + + let initial_count_flag = PackedFlagUint::new_raw(initially_static, 1); + let count_flag: AtomicUsize = AtomicUsize::new(initial_count_flag.encoded_value()); + core::ptr::addr_of_mut!((*ptr).count_flag).write(count_flag); debug_assert_eq!( (ptr as *const u8).wrapping_add(OFFSET_DATA), (*ptr).data.as_ptr(), ); - debug_assert_eq!(&(*ptr).data as *const _ as *const u8, (*ptr).data.as_ptr()); Ok(NonNull::new_unchecked(ptr)) } @@ -931,9 +1085,10 @@ impl ThinInner { // returns `Err(Some(l))` if we failed to allocate that layout, and // `Err(None)` for integer overflow when computing layout. - fn try_allocate(data: &str) -> Result, Option> { + #[inline] + fn try_allocate(data: &str, initially_static: bool) -> Result, Option> { // Allocate a enough space to hold the given string - let uninit = Self::try_allocate_uninit(data.len())?; + let uninit = Self::try_allocate_uninit(data.len(), initially_static)?; // Copy the given string into the allocation unsafe { core::ptr::copy_nonoverlapping(data.as_ptr(), Self::data_ptr(uninit), data.len()) } @@ -941,16 +1096,16 @@ impl ThinInner { Ok(uninit) } #[inline] - unsafe fn get_len_flags(p: *const ThinInner) -> LenFlags { + unsafe fn get_len_flag(p: *const ThinInner) -> PackedFlagUint { debug_assert_eq!(OFFSET_LENFLAGS, 0); *p.cast() } #[cold] unsafe fn destroy_cold(p: *mut ThinInner) { - let lf = Self::get_len_flags(p); - debug_assert!(!lf.is_static()); - let len = lf.len(); + let lf = Self::get_len_flag(p); + let (is_static, len) = (lf.flag_part(), lf.uint_part()); + debug_assert!(!is_static); let layout = { let size = len + OFFSET_DATA; let align = align_of::(); @@ -972,7 +1127,7 @@ impl From<&str> for ArcStr { if s.is_empty() { Self::new() } else { - Self(ThinInner::allocate(s)) + Self(ThinInner::allocate(s, false)) } } } @@ -1210,60 +1365,47 @@ impl core::str::FromStr for ArcStr { } } -#[cold] -#[inline(never)] -#[cfg(not(feature = "std"))] -fn abort() -> ! { - struct PanicOnDrop; - impl Drop for PanicOnDrop { - fn drop(&mut self) { - panic!("fatal error: second panic") - } - } - let _double_panicer = PanicOnDrop; - panic!("fatal error: aborting via double panic"); -} - -#[cfg(feature = "std")] -use std::process::abort; - #[cfg(test)] +#[cfg(not(msrv))] // core::mem::offset_of! isn't stable in our MSRV mod test { use super::*; fn sasi_layout_check() { assert!(align_of::>() >= 8); assert_eq!( - memoffset::offset_of!(StaticArcStrInner, count), - OFFSET_STRONGCOUNT + core::mem::offset_of!(StaticArcStrInner, count_flag), + OFFSET_COUNTFLAGS ); assert_eq!( - memoffset::offset_of!(StaticArcStrInner, len_flags), + core::mem::offset_of!(StaticArcStrInner, len_flag), OFFSET_LENFLAGS ); assert_eq!( - memoffset::offset_of!(StaticArcStrInner, data), + core::mem::offset_of!(StaticArcStrInner, data), OFFSET_DATA ); assert_eq!( - memoffset::offset_of!(ThinInner, strong), - memoffset::offset_of!(StaticArcStrInner::, count), + core::mem::offset_of!(ThinInner, count_flag), + core::mem::offset_of!(StaticArcStrInner::, count_flag), ); assert_eq!( - memoffset::offset_of!(ThinInner, len_flags), - memoffset::offset_of!(StaticArcStrInner::, len_flags), + core::mem::offset_of!(ThinInner, len_flag), + core::mem::offset_of!(StaticArcStrInner::, len_flag), ); assert_eq!( - memoffset::offset_of!(ThinInner, data), - memoffset::offset_of!(StaticArcStrInner::, data), + core::mem::offset_of!(ThinInner, data), + core::mem::offset_of!(StaticArcStrInner::, data), ); } #[test] fn verify_type_pun_offsets_sasi_big_bufs() { - assert_eq!(memoffset::offset_of!(ThinInner, strong), OFFSET_STRONGCOUNT); - assert_eq!(memoffset::offset_of!(ThinInner, len_flags), OFFSET_LENFLAGS); - assert_eq!(memoffset::offset_of!(ThinInner, data), OFFSET_DATA); + assert_eq!( + core::mem::offset_of!(ThinInner, count_flag), + OFFSET_COUNTFLAGS, + ); + assert_eq!(core::mem::offset_of!(ThinInner, len_flag), OFFSET_LENFLAGS); + assert_eq!(core::mem::offset_of!(ThinInner, data), OFFSET_DATA); assert!(align_of::() >= 8); @@ -1313,11 +1455,12 @@ mod loomtest { #[test] fn drop_timing() { loom::model(|| { - let a1 = (0..5) - .map(|i| ArcStr::from(alloc::format!("s{}", i))) - .cycle() - .take(10) - .collect::>(); + let a1 = alloc::vec![ + ArcStr::from("s1"), + ArcStr::from("s2"), + ArcStr::from("s3"), + ArcStr::from("s4"), + ]; let a2 = a1.clone(); let t1 = thread::spawn(move || { diff --git a/src/lib.rs b/src/lib.rs index 4f0cae3..0bbd28e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -64,6 +64,8 @@ #![cfg_attr(not(feature = "std"), no_std)] #![deny(missing_docs)] #![allow(unknown_lints)] +// for `cfg(loom)` and such -- I don't want to add a build.rs for this. +#![allow(unexpected_cfgs)] #[doc(hidden)] pub extern crate alloc; diff --git a/src/mac.rs b/src/mac.rs index bd0e89c..22378ad 100644 --- a/src/mac.rs +++ b/src/mac.rs @@ -32,18 +32,24 @@ macro_rules! literal { // (note that consts in macros dont have hygene the way let does). const __TEXT: &$crate::_private::str = $text; { + #[allow(clippy::declare_interior_mutable_const)] const SI: &$crate::_private::StaticArcStrInner<[$crate::_private::u8; __TEXT.len()]> = unsafe { &$crate::_private::StaticArcStrInner { - len_flags: __TEXT.len() << 1, - count: 0, + len_flag: match $crate::_private::StaticArcStrInner::<[$crate::_private::u8; __TEXT.len()]>::encode_len(__TEXT.len()) { + Some(len) => len, + None => $crate::core::panic!("impossibly long length") + }, + count_flag: $crate::_private::StaticArcStrInner::<[$crate::_private::u8; __TEXT.len()]>::STATIC_COUNT_VALUE, // See comment for `_private::ConstPtrDeref` for what the hell's // going on here. data: *$crate::_private::ConstPtrDeref::<[$crate::_private::u8; __TEXT.len()]> { p: __TEXT.as_ptr(), } .a, + // data: __TEXT.as_ptr().cast::<[$crate::_private::u8; __TEXT.len()]>().read(), } }; + #[allow(clippy::declare_interior_mutable_const)] const S: $crate::ArcStr = unsafe { $crate::ArcStr::_private_new_from_static_data(SI) }; S } diff --git a/tests/arc_str.rs b/tests/arc_str.rs index 06430c5..57f010d 100644 --- a/tests/arc_str.rs +++ b/tests/arc_str.rs @@ -307,22 +307,22 @@ fn test_froms_more() { let cow: Cow<'_, str> = Owned("abcd".into()); assert_eq!(ArcStr::from(cow), "abcd"); - let cow: Option> = Some(&arc).map(Cow::from); + let cow: Option> = Some(Cow::from(&arc)); assert_eq!(cow.as_deref(), Some("asdf")); - let cow: Option> = Some(arc).map(Cow::from); + let cow: Option> = Some(Cow::from(arc)); assert!(matches!(cow, Some(Cow::Owned(_)))); assert_eq!(cow.as_deref(), Some("asdf")); let st = { arcstr::literal!("static should borrow") }; { - let cow: Option> = Some(st.clone()).map(Cow::from); + let cow: Option> = Some(Cow::from(st.clone())); assert!(matches!(cow, Some(Cow::Borrowed(_)))); assert_eq!(cow.as_deref(), Some("static should borrow")); } // works with any lifetime { - let cow: Option> = Some(st.clone()).map(Cow::from); + let cow: Option> = Some(Cow::from(st.clone())); assert!(matches!(cow, Some(Cow::Borrowed(_)))); assert_eq!(cow.as_deref(), Some("static should borrow")); } diff --git a/tests/substr.rs b/tests/substr.rs index 423c57b..ef7db98 100644 --- a/tests/substr.rs +++ b/tests/substr.rs @@ -269,23 +269,23 @@ fn test_cow() { let cow: Cow<'_, str> = Owned("abcd".into()); assert_eq!(Substr::from(cow), "abcd"); let sub = ArcStr::from("XXasdfYY").substr(2..6); - let cow: Option> = Some(&sub).map(Cow::from); + let cow: Option> = Some(Cow::from(&sub)); assert_eq!(cow.as_deref(), Some("asdf")); - let cow: Option> = Some(sub).map(Cow::from); + let cow: Option> = Some(Cow::from(sub)); assert!(matches!(cow, Some(Cow::Owned(_)))); assert_eq!(cow.as_deref(), Some("asdf")); let st = { arcstr::literal!("_static should borrow_") }; let ss = st.substr(1..st.len() - 1); { - let cow: Option> = Some(ss.clone()).map(Cow::from); + let cow: Option> = Some(Cow::from(ss.clone())); assert!(matches!(cow, Some(Cow::Borrowed(_)))); assert_eq!(cow.as_deref(), Some("static should borrow")); } // works with any lifetime { - let cow: Option> = Some(ss.clone()).map(Cow::from); + let cow: Option> = Some(Cow::from(ss.clone())); assert!(matches!(cow, Some(Cow::Borrowed(_)))); assert_eq!(cow.as_deref(), Some("static should borrow")); }