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

Support const alias_tensor_instance #204

Closed
lvella opened this issue Aug 26, 2016 · 12 comments
Closed

Support const alias_tensor_instance #204

lvella opened this issue Aug 26, 2016 · 12 comments

Comments

@lvella
Copy link
Contributor

lvella commented Aug 26, 2016

It would be nice if alias_tensor class had support for const tensor&, something like:

        alias_tensor_const_instance operator() (
            const tensor& t,
            size_t offset
        );

Is it feasible?

@davisking
Copy link
Owner

Not in any clean way since alias_tensor_const_instance needs to inherit from tensor. So you will be able to pass it to functions that take a tensor& and it will compile. We could make mutating functions throw an error, but I'm not sure that's any better than const casting and using the existing alias tensor.

You will also have a problem where an alias_tensor_const_instance is unusable unless it is itself declared const because the non-const members will be selected automatically when they are called, leading to exceptions being thrown.

I'm open to ideas to make it workable but it's not super clear how to do it without a bunch of problems.

@lvella
Copy link
Contributor Author

lvella commented Aug 26, 2016

What if: return simply a const alias_tensor_instance and defines a C++11 const move constructor for that class? Never seem it done, a const klass&& other parameter, but should work, right?

@davisking
Copy link
Owner

davisking commented Aug 26, 2016 via email

@lvella
Copy link
Contributor Author

lvella commented Aug 26, 2016

Not const constructors, const rvalue reference constructors. But on a second thought, this is a recipe for a disaster...

class PointerHolder
{
public:
    PointerHolder(const PointerHolder&& other)
    {
        ptr = other.ptr;

        /* Can't be done, unless ptr is mutable: */
        //other.ptr = nullptr;
    }

private:
    PointerHolder(int *ptr):
        ptr(ptr)
    {}

    friend const PointerHolder pointer_holder_creator(int *ptr);

    int *ptr;
};

const PointerHolder pointer_holder_creator(int *ptr)
{
    const PointerHolder ret(ptr);
    return ret;
}


int main()
{
    int *a = new int[42];

    {
        const PointerHolder h(pointer_holder_creator(a));

        h; // ...
    }

    delete[] a;
}

I don't know how to make it work on classes with non-trivial destructors without using mutable, because you wouldn't be able to change the state of the rvalue referenced object to a valid destructible state. One of possibly many problems is that one would be allowed to create a non-const version of a const object, like:

    PointerHolder const_removed_h(std::move(h));

@davisking
Copy link
Owner

davisking commented Aug 26, 2016 via email

@lvella
Copy link
Contributor Author

lvella commented Aug 26, 2016

Hope you don't mind, I've asked on StackOverflow:
https://stackoverflow.com/questions/39168434/how-to-do-a-factory-of-const-objects

@lvella
Copy link
Contributor Author

lvella commented Aug 26, 2016

I deleted my previous comment because I think I didn't really understood the problem, and the solution proposed was wrong. The suggestion given by Yakk on Stack Overflow seems to work like this:

class alias_tensor_const_holder
{
public:
    const tensor& get() const {
        return t;
    }
private:
    const alias_tensor_instance t;
}

/* ... */

alias_tensor_const_holder operator() (
            const tensor& t,
            size_t offset
);

The copy to non-const is solved by hiding the object privately, and never exposing it directly, but only through a const tensor& interface, so if the user makes a copy, it is a deep copy.

@davisking
Copy link
Owner

davisking commented Aug 26, 2016

Very good ideas on stack overflow 👍

That's definitely a good solution. Adding an implicit conversion in addition to a get() makes it a really nice solution. I'll add this to dlib. Or you can make a pull request with the changes if you like :)

@lvella
Copy link
Contributor Author

lvella commented Aug 27, 2016

Although I am not sure how to build a const alias_tensor_instance from a const tensor&, I'll try.

@davisking
Copy link
Owner

You can do the conversion like this: const_cast<tensor&>(t) and pass that to the existing operator() to get the alias_tensor_instance needed. I can add the code if you would rather me to it, it's no trouble.

@lvella
Copy link
Contributor Author

lvella commented Aug 27, 2016

Please do it, or I'm afraid I'll waste effort trying to match the code style only to be rejected in the end. I was trying to do without using const_cats<> but it seems to be impossible...

@davisking
Copy link
Owner

I just pushed the code for this. So you should be able to use const aliasing tensors now. Thanks for suggesting the idea :)

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

2 participants