-
-
Notifications
You must be signed in to change notification settings - Fork 705
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
Fix Issue 9636: null initialization for std.typecons.Nullable #2593
Conversation
|
The spirit of the pull looks good to me. But now, for some bike shedding: enum DefaultNullable = DefaultNullableImpl.init;That's not a type, so lower case please: private alias DefaultNullableImpl = Typedef!(byte, 0, "std.typecons.DefaultNullable");Why the whole Typedef? Just: Finally, I don't like the word "DefaultNullable". There's nothing "default" about it. I'd go for |
|
|
Thats... actually a good point. |
(Citing @monarchdodra from bugzilla)
If the wrapped type has a null state, it's pointless to use Nullable with it. I agree that it's possible, however the point of Nullable is to allow one to have a value in an "invalid"/"uninitialized" state which is exactly what null is for pointers.
Looking at the issue (9636), nothing more than a syntactic sugar for default parameter. I think we should rather disallow pointers for Nullable (unless someone can cite a valid use case) and resurrect something like #1356 . |
|
@Geod24: >If the wrapped type has a null state, it's pointless to use Nullable with it. This is not true. Sometimes you want to avoid raw null pointers with a Nullable!(int*, null). It uses the same memory as a raw pointer, but it's guarded by asserts. @JakobOvrum: >Nullable.init is already in the null state, and it's also default-constructable. So why all this? Do you mean I should write code like this? I find it not acceptable and I'd like some shorter way to write it. |
|
We could make |
|
@JakobOvrum: >We could make Nullable(T) alias to T when T is already nullable, Please let's keep the semantics of Nullable as much as possible clean. A clean semantics avoids tons of problems later. |
Semantics are cleaner without the nasty double null situation. |
Just use an alias. Are you seriously using
Furthermore, Nullable can be also be to differentiate between "default state" and "not yet specified state". If my function taking a
Do we even have a |
If somebody writes |
Attaching an arbitrary boolean state variable should not be the scope of
Just
What if they write |
|
@JakobOvrum> Semantics are cleaner without the nasty double null situation. It's not very different than using Nullable!(Nullable!int). If you introduce implicit type conversions for reference types, you are introducing more complexity in Nullable, so you are making its semantics more complex (and indeed you have to add more documentation to explain its behavour in ddoc comments). For me more complexity and special cases mean a less clear semantics. |
I already said in what I quoted: Then it's better to have consistent state, regardless of what T is. |
It does have consistent state regardless of |
|
@monarchdodra >Just use an alias. As you see in Issue 9636 I am currently using an alias. But I'd like a bit of syntax sugar to avoid the alias (also because the alias needs to be in the outer scope, like at module level). If you think such usage is not worth the added complexity, than this is a judgement call :-) @monarchdodra> Type t = Type.init unacceptable... When "Type" is something longer like "immutable int[4]" or even longer, then it's not DRY and it becomes quite long. |
|
It seems I have to fight "tooths and nails" for every little thing I'd like added to Phobos :-) |
OK, I agree. But how is that specific to |
That's probably a better idea. I'll change it to a struct.
How would that work?
Well, if you want to be technical, it's the very definition of a default Nullable, as the constructor that takes |
As Bearophile said, code like the following: It would be annoying to introduce an alias for a type that you're only going to use once, and often the type wrapped by Nullable does not have its own null state (which is probably why you're using Nullable in the first place), so there is no way to provide default initialization to null like above. Changing Nullable now to use typeof(null) is the obvious solution, but that may break code now. I agree that it is a design mistake that Nullable does not alias itself to T if T is a nullable type itself. |
|
@monarchdodra >But how is that specific to Nullable ? For the use case explained in Issue 9636 the problems are specific for Nullable. I haven't found similar problems with other structs/typecons. |
Anything with a "not yet initialized" state comes to mind. And even then, for everything else, you are still just writing a variation of Frankly, if it's not |
That's a nice trick to know. I also note it doesn't require a separate null state. So you're right, we shouldn't forbid nullable types for Nullable.
Agree. Although why should we keep
The default state for pointer is "not yet specified", as |
Um what? The C standard library would disagree with you.
Pointers are but one way to implement nullable on a type. Both |
auto getNullable!T(T t)
{
auto a = Nullable!T(t);
assert(!a.isNull); //or is it...?
return a;
} |
|
@monarchdodra, indeed that demonstrates that in the current version one can always rely on the Again, attaching an arbitrary boolean state to a type should not be within the scope of |
That ship has sailed. Changing it now will potentially break code. |
Phobos has a number of shoddy or incomplete types or modules, and I think Anyway, I'm fine with judging this PR against the current (broken) design, I admit there is no need to expand the scope of this pull request. Are there any other situations apart from default arguments where One thing to note is that while it doesn't save on typing, one can opt to use overloading to achieve the same effect but with arguably nicer documentation than the void foo(Nullable!(immutable int[4]) items);
void foo();Secondly, although this one will certainly vary, needing to use a complicated type for |
Arguably, I think the issue you are showing here is abusive use of
|
|
@monarchdodra: >Anything with a "not yet initialized" state comes to mind. RefCounted, random generators, or pretty much any container. With the current design of std.random module, the random generators can't be passed by value, but only by ref (or by pointer), and you can't add a default value to a ref argument. So this is an invalid example. If the problem is really general as you say, then are you suggesting a general solution, some "defaultInitialized" that works with them all (including Nullable)? |
|
@JakobOvrum: >One thing to note is that while it doesn't save on typing, one can opt to use overloading to achieve the same effect In my use case the function had one nullable argument, and four nullable arguments that should default to null. You can't use overloading here, it makes the code awful. But I agree this is an uncommon use case. |
I was not actually suggesting a general solution, as I don't really think there is a problem here. At least, not a problem that warrants the current change suggested here.
That's called T t = defaultInit;That's indeed something I think would be a more generic "solution". But even then, I wouldn't be sure an extra concept is warranted here :/ |
|
This is a simple change. Can we either merge it or close it? There's no good alternative to the problem described issue 9636, when you have a long type name and don't want to introduce an external alias. Or, we can close this, and I'll create another pull making Or, I can change |
Nullable!(int*) a = null;
assert(a.isNull);
int* p = null;
Nullable!(int*) b = p;
assert(!b.isNull);Let's not do that. It causes
|
What I mean is, removing double-null from Nullable and allowing assignment of null or construction from null of Nullable to do what the proposed defaultNullable does. |
Well that's exactly what I've been suggesting. Once you remove the double null from |
|
Yes, and that will be a breaking change. |
|
Ping |
|
I would be in favor of closing it until we come to a decision here. |
|
This is always going to be a problem unless we want to break Nullable, but I guess I'll have to close this for now. |
https://issues.dlang.org/show_bug.cgi?id=9636
For now, it's only allowed to use DefaultNullable on initialization. Nullable already has a Nullify function, so it's not as useful to also enable assigning DefaultNullable to a Nullable to nullify it.
If possible, this should be merged before #2587, so I can then amend that PR to print "DefaultNullable" when Nullable is internally "null", so as not to cause confusion if the wrapped type also has a null state.