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

Additional Scope and Ref Functions #560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silvertakana
Copy link

There isn't an issue. This is just an addition for more functionality

@JonathanHiggs
Copy link
Contributor

JonathanHiggs commented Jul 6, 2022

GetRef is bugged, super easy to end up in undefined behaviour, either with a local variable that has been deleted while still in scope, or a ref that points to memory that has already been deleted (see examples below)

The issue is that passing in a T& implies that the caller still owns the memory, but you are passing that into shared_ptr which implies it is taking over ownership of the memory

struct Foo
{
    bool& deleted;
    ~Foo() { deleted = true; }
};


TEST(RefTest, RefDeletesInScopeValue)
{
    bool deleted = false;
    Foo foo { deleted };

    {
        auto ref = GetRef<Foo>(foo);
    }

    // foo is still in scope but has been deleted by the ref going out of scope
    EXPECT_FALSE(deleted);
}

TEST(RefTest, RefPointsToReclaimedMemory)
{
    bool deleted = false;
    auto returnRef = [&]() -> Ref<Foo> {
        Foo foo{ deleted };
        return GetRef<Foo>(foo);
    };

    auto ref = returnRef();

    // foo was deleted when it went out of scope so ref points to reclaimed memory
    EXPECT_FALSE(deleted);
}

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

Successfully merging this pull request may close these issues.

2 participants