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

Resolve const #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

shoheikuni
Copy link

Hi. As well as JonathanHiggs in #54, I have a need to resolve const T types.

I tried his code and it allowed me to resolve an constructor argument of std::shared_ptr<const T>, but I couldn't compile simply container->resolve<const T>().

The necessary behavior is that a std::shared_ptr<T> is internally created when const T is required to resolve, since std::shared_ptr<T> is convertible to std::shared_ptr<const T>.
I've fixed some lines in ComponentContext::tryToRegisterType() to register std::remove_const_t<T> instead of T itself.

I hope I didn't make a mistake nor misunderstanding.

Thanks.

@JonathanHiggs
Copy link

@shoheikuni My question with this approach vs #54 is whether a registration of const T would then be resolvable as non-const which might be counter intuitive and break any intended immutability

@shoheikuni
Copy link
Author

@JonathanHiggs Hi. My proposal is just for resolving const T when T is registered.
You can't register const T in my approach (and in the original codebase as well), so you don't need to concern about breaking immutability.
That is:

        ContainerBuilder builder;
        builder.registerType<Foo>(); // If you register Foo,
        auto container = builder.build();
        auto foo = container->resolve<Foo>(); // then you can resolve Foo
        auto cfoo = container->resolve<const Foo>(); // and const Foo.

        //builder.registerType<const Foo>(); // But you can't do this.

My proposal would not be enough If someone really wanted to register const T, but I'm not sure of such a need.

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