Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[cfg(feature = "debug_atomic_refcell")] ? #16

Open
Des-Nerger opened this issue Jun 16, 2022 · 2 comments
Open

#[cfg(feature = "debug_atomic_refcell")] ? #16

Des-Nerger opened this issue Jun 16, 2022 · 2 comments

Comments

@Des-Nerger
Copy link

Des-Nerger commented Jun 16, 2022

In the original RefCell implementation there is a cfg feature called "debug_refcell", which in case of a panic allows to see where a RefCell was first borrowed:

pub struct RefCell<T: ?Sized> {
    borrow: Cell<BorrowFlag>,
    // Stores the location of the earliest currently active borrow.
    // This gets updated whenever we go from having zero borrows
    // to having a single borrow. When a borrow occurs, this gets included
    // in the generated `BorrowError/`BorrowMutError`
    #[cfg(feature = "debug_refcell")]
    borrowed_at: Cell<Option<&'static crate::panic::Location<'static>>>,
    value: UnsafeCell<T>,
}

I read that you stripped RefCell of everything that conflicted multi-thread environment. Is it because of this that the debug feature was removed? Is it hard to reimplement it back? Pushing borrow checking into runtime leaves us with less info to investigate things. This leads projects like accountable-refcell to store the whole stack trace per RefCell. Well, that much of an overhead is probably overkill, but to have the minimal "borrowed_at" info is an essential thing to have for debug builds, don't you think?🤔

@bholley
Copy link
Owner

bholley commented Jun 16, 2022

That does seem like a useful feature. That said, adding conditionally-compiled debug code to the core borrowing logic would make it somewhat more complex, and I've been trying hard to keep it a simple as possible.

If someone wants to implement this feature, I'd be happy to take it on a branch in this repo (without any conditional compilation). That way, anyone debugging such a failure could simply put a [patch] in their Cargo.toml to point to that branch, which is not substantially more difficult then modifying Cargo.toml to enable a debug_refcell feature.

@cehteh
Copy link

cehteh commented May 1, 2023

instead dedicated feature this could be just enabled with: #[cfg(debug_assertions)]

I see its arguable to add more complexity, but personally I would be in favor to add it, as it can be of sufficient value when debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants