joboccara / NamedType

Implementation of strong types in C++
MIT License
769 stars 84 forks source link

non-const reference to underlying type #11

Open arvidn opened 6 years ago

arvidn commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.