Move std.container.make to std.conv and expand on it #756

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Member

jmdavis commented Aug 22, 2012

This pull request does the following:

  1. Move std.container.make to std.conv and makes it work with all types rather than just classes and structs (while aliasing and scheduling for deprecation std.container.make).
  2. Add std.conv.makeNew which is the same as make except that it always uses new.
  3. Add std.conv.makeArray, which is a specialized version of make for arrays with the idea that it makes it so that you can construct arrays of integral types smaller than int without all of the casting that you have to do with integer literals (makeArray does the casting for you).
  4. Add std.conv.makeStaticArray, which is the same as makeArray except that it's for static arrays. It also has the benefit of inferring the length of the array. Both it and make also avoid heap allocations when initializing static arrays.

These functions should make generic type construction easier in the cases where generic type construction makes sense as well as clean up constructing arrays with integer literals. I would think that makeNew (and possibly makeArray) would be the same as what custom allocators would have for constructing types (though presumably custom allocators would use the name make or alloc rather than makeNew), but I guess that that would depend on the exact design of custom allocators, which hasn't been finalized yet. However, these functions should be useful regardless of what happens with custom allocators, and they're an extension of what we already have with std.container.make.

std/container.d
@@ -828,23 +828,14 @@ unittest {
}
/**
+$(RED Scheduled for deprecation in February 2013.
+ Please use $(XREF typecons, make) instead.)
@andralex

andralex Sep 2, 2012

Owner

We shouldn't deprecate this, it's gratuitous. Just alias it away.

@jmdavis

jmdavis Sep 2, 2012

Member

I can leave it, but it's a pointless alias, and we generally try to not have those. The only reason that I see to leave it is that it will break some code to remove it, and we're trying to avoid that, which may make it worth keeping, but ideally, it wouldn't be left here.

std/typecons.d
@@ -3324,3 +3324,498 @@ unittest
assert(flag1 == Yes.abc);
}
+
+/++
+ Constructs T generically. This function is mainly for eliminating the
@andralex

andralex Sep 2, 2012

Owner

std.typecons is not the appropriate place for this. "Typecons" stands in for "type constructors", i.e. it creates new types. This function creates new values of already-existing types.

@alexrp

alexrp Sep 2, 2012

Member

I think std.conv might be the place to put these, seeing as emplace is there.

@andralex

andralex Sep 2, 2012

Owner

std.conv sounds like a good place

@jmdavis

jmdavis Sep 2, 2012

Member

I would have considered this to fall directly into the purview type construction, but I wouldn't have thought of type construction as creating new types in the way that you clearly do. I can certainly move it to std.conv though.

jmdavis added some commits Aug 22, 2012

Moved make from std.container to std.conv.
It's also been expanded to work with all types rather than just structs
and classes.
Added makeNew to std.conv.
It's like make except that it specifically constructs objects with new.
Added makeStaticArray to std.conv.
Like makeArray, it makes it easy to construct static arrays of integral
types smaller than int without explicity casting, but it also has the
benefit of inferring the length of the array.
Added makeArray to std.conv.
It makes it easier to construct arrays of integral types smaller than
int, which typically requires a lot of casting when using literals.
Member

jmdavis commented Sep 3, 2012

Okay. I moved the functions into std.conv and rebased. Alos, while std.container.make is still an alias, I removed the message about it being scheduled for deprecation.

Owner

andralex commented Sep 26, 2012

So what is the motivation for this? The original motivation of std.container.make was to create containers with variadic arguments in a somewhat uniform fashion. As far as I understand the generalization covers class objects and arrays, and that's pretty much it.

All told, with documentation and unit tests, the whole thing has ballooned from literally three lines to 500 lines. I don't find any fault with any line, but I don't have to, because I find fault with the overall design. There's absolutely no way during my lifetime I'll approve a design that has this terrible cost/utility ratio. I apologize for my being blunt. I will close this now.

@andralex andralex closed this Sep 26, 2012

Member

jmdavis commented Sep 26, 2012

make is needed for dealing with classes vs structs. It does this already in std.container (though for some reason it restricts itself to constructing types with at least one argument). Expanding it to work with types in general seems to me to be sensible and potentially useful for generic code. std.container.make does not currently work with anything other than structs and classes. Also, moving it to somewhere other than std.container makes sense in that it doesn't really have anything to do with containers beyond the fact that it's a common case where you don't want to have to worry about whether you're dealing with a struct or a class.

makeNew is needed to be able to sanely deal with the differences between creating different types on the heap. This is particularly critical for scalar types, because there is no way in the language to allocate scalar types on the heap and initialize them at the same time. That's annoying enough with mutable types, but it's arguably crippling with const and immutable ones because of all the casts required to actually do it properly. If you could do something like new int(5) or new immutable(float)(1.1), then it wouldn't be so bad, but you can't.

makeArray is here because of how insanely annoying it is to create array literals of anything smaller than int. You literally have to cast every element in an array literal to get it to be a ubyte[] or ushort[] or any array type with elements smaller than int unless you're directly assigning it to an array with elements of the smaller type. And actually, you have the same problem if you want larger elements or if you're mixing various class types together. Basically to have an array with anything other than what the elements are inferred to be requires casting all of the elements individually (and if they don't all infer to the same type, you need to cast regardless). makeNew does all of that casting for you, making it so that you can actually have array literals of other types without making your code really ugly.

makeStaticArray is here to do the same thing as makeArray but for static arrays with the added benefit of inferring the size of the array for you, which you can't do with the language either.

Owner

andralex commented Sep 26, 2012

I understood the motivation first time I looked at the code. But I don't agree with the price tag. So the challenge is how to do this all in 50 lines instead of 500.

Member

jmdavis commented Sep 26, 2012

And 4/5ths of that is documentation and unit tests. No offense, but you seem to react negatively to anything that actually tries to do a good job at unit testing simply because of the number of lines of code that those tests take up, which is going to lead to more poorly tested code and thus buggier code.

Maybe by switching to using static ifs instead of function overloads or by reformatting the code differently, the number of lines of code could be reduced but not really the amount of actual code. To do what this is doing, it needs about as much code as is there. Certainly, if you want it in 50 lines or less including documentation and unit tests, I think that that's completely unreasonable. Sure, the code itself could be reduced to 50 lines if you remove half of the functionality, but then you're losing half the functionality, and it sounds like in that case, you'd be calling for it to take up half the space.

I really don't think that this is any worse than std.container.make as far as functionality added per line of actual code goes. It's just that it provides more functionality and actually has tests for it.

@ghost

ghost commented Sep 26, 2012

I'd replace bool check with something more descriptive or even use an enum so the call site is self-documenting:

return _makeStaticArray!(ElementEncodingType!T, ConvCheck.yes)(args);

vs:

return _makeStaticArray!(ElementEncodingType!T, true)(args);

The //No heap allocations comments in the unittest sections can be removed, the examples already state no heap allocation.

Why is explicit casting only done for makeArray/makeStaticArray functions and not make functions? This sounds like something that should be customizable via a CT/RT option.

@ghost

ghost commented Sep 26, 2012

Also there's no need for duplicate examples, e.g.:

ushort[4] sArr1 = makeStaticArray!ushort(4, 9, 22, 7);
assert(sArr1 == [4, 9, 22, 7]);

//No heap allocations
byte[3] sArr2 = makeStaticArray!byte(1, 2, 3);
assert(sArr2 == [1, 2, 3]);

The first example explains everything, the second one is just noise.

@ghost

ghost commented Sep 26, 2012

Your error handler prints the wrong message, see:

auto x = make!(ushort[2])(0, 0);
    test.d(39): Error: static assert  "Argument# 0 is not implicitly convertable to void"
    test.d(53):        instantiated from here: _makeStaticArray!(ushort,true,int,int)
    test.d(61):        instantiated from here: make!(ushort[2u],int,int)

In the call to Format!() don't use ElementEncodingType!T.stringof since T is already the element type, use T instead.

@ghost

ghost commented Sep 26, 2012

I would really avoid writing opEquals in any classes for comparison purposes. First off it's not correct to begin with, for example you're not checking if after calling auto rhs = cast(C)obj; whether rhs is null. Also, if the signature of opEquals changes in the future (e.g. maybe it will require const) you're going to have to fix the example, which means extra wasted time on fixing unittests. Just define the comparison inline:

assert(sArr4.i == arr4.i && sArr4.f == arr4.f);

In fact remove the underscores and store only 2 fields (or even 1, it's not like 3 fields is going to make the tests any better, it's only visual noise).

    auto arr4 = make!(const C[])(new C(9, 5, "h"), new const(C)(1, 2, "w"));
    assert(arr4 == [new const(C)(9, 5, "h"), new const(C)(1, 2, "w")]);

can be replaced with:

auto arr4 = make!(const C[])(new C(9, 5));
assert(arr4[0].i == 9 && arr4[0].f == 5);

No need for 3+ fields. Also I'm very much against doing any sort of floating-point comparisons in unittest code which isn't specifically meant for testing exactly that.

The same could be said about the array comparisons, 2-length array literals are good enough, no need for 5 elements which only creates visual clutter.

Member

jmdavis commented Sep 29, 2012

I'd replace bool check with something more descriptive or even use an enum so the call site is self-documenting:

It seems like overkill to create an enum like that for stuff which is completely private to a module.

Why is explicit casting only done for makeArray/makeStaticArray functions and not make functions?

Because in general, casting is a very blunt instrument and very error-prone. And making the casting work for anything other than arrays (e.g. constructor arguments) is very complicated because of all of the compile-time reflection which you have to do.

This sounds like something that should be customizable via a CT/RT option.

I considered that, but that would just make it overly complicated IMHO. The only reason the casting is there at all is because it's so annoying to create an array literal of any types which aren't automatically inferred correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment