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

non-const reference to underlying type #11

Open
arvidn opened this issue Mar 25, 2018 · 8 comments
Open

non-const reference to underlying type #11

arvidn opened this issue Mar 25, 2018 · 8 comments

Comments

@arvidn
Copy link
Contributor

arvidn commented Mar 25, 2018

It seems like having a get() overload return a non-const reference to the underlying type is a pretty big safety risk, essentially circumventing all the type restrictions put in place with NamedType.

My understanding is that this function is quite widely used by the Skills, which makes sense that they would have access to a non-const reference to the underlying object. But perhaps it would make sense to make the non-const get() overload be protected, so it's only available to the skills, and not part of the public API.

@joboccara
Copy link
Owner

Hi Arvid,
The intent for the non-const get() method (and the const one as well), was to allow code using NamedType to operate with code that doesn't. For the non-const type, one example would be an assignment to an underlying type. We could argue that we could wrap the value to assign into a NamedType but that could incur a copy, which may or may not be desirable.
Does this seem reasonable to you?

@arvidn
Copy link
Contributor Author

arvidn commented Mar 26, 2018

I take you specifically refer to the case of traditional code taking a parameter by non-const reference, as an out-parameter. If it's just a matter of casting the result of non-typesafe function into the correct type, one can just use the constructor.

Since out-parameters are being phased out (and generally recommended against) in favor of multiple return values. In my mind, the cost of introducing this loophole in the type-safety is not worth the case of supporting out-parameters. Requiring a copy for out-parameters seem like a reasonable disincentive to use them.

I suppose the other use case would be supporting std::tie() with a function returning the untyped value.

@joboccara
Copy link
Owner

Yep, totally in line with you on discouraging out-parameters. The case I was thinking about was like

int f();
using MyInt = NamedType<int, struct MyIntTag>;
MyInt x;
...
x.get() = f();

Does this look like a valid use case to you?

@arvidn
Copy link
Contributor Author

arvidn commented Mar 27, 2018

I see. In my mind, that should be typed as:

x = MyInt{f()};

But you won't really get all the type-safety benefits until f() returns MyInt.

@FlorianWolters
Copy link

I agree with @arvidn, getting rid of the non-const get function leads to cleaner code, imo. Strong types should not be mutable by directly setting the value of the underlying data type.

// Standard Library
#include <iostream>

// NamedType
#include <named_type.hpp>

using Width = fluent::NamedType<double, struct WidthTag>;

double calculateWidth() {
  return 42.0;
}

int main() {
  Width bad_width{0};
  bad_width.get() = calculateWidth();
  std::cout << bad_width.get() << '\n'; // 42.0, WTF!?

  Width good_width{calculateWidth()};
  std::cout << good_width.get() << '\n'; // 42.0, OK!
}

@joboccara
Copy link
Owner

Hello Florian,

I get your point. Would it be good to have it as an opt-in skill then? Because having this setter can be handy in some cases, like for introducing strong types in an existing piece of code that performs sets.
Or maybe rather a set() method? Or a set opt-in.

Jonathan

@arvidn
Copy link
Contributor Author

arvidn commented Sep 7, 2018

my opinion is that along the edges of introducing strong types, it's a feature for the conversions to be verbose and explicit. The two cases I can think of is where a weak type is returned normally and where it's returned as an out-parameter. I think the normal way to deal with that should be:

Width x;
x = Width(calculateWidth());
Width x;
double temp;
calculateWidth(&temp);
x = Width(tmp);

Partly because the out-param case should be a bit extra penalised (because it represents two separate fixes that need to be made).

@viboes
Copy link

viboes commented Sep 9, 2018

If the user want to add the operator=(UT) from the underlying type, she can do it as herself, but this is not an operation every strong type should have.

I believe that the access to the underlying type should return a reference/const reference, but the name shouldn't be something friendly. I have used a backdoor idiom to state clearly that the mixin are using something that in principle can be used only by the mixins.

I access it as st._backdoor()._underlying()

If the user wants to declare a get function that return as well a non const reference it is up to the user, or maybe you want to provide a GetAble mixin.

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

4 participants