Turn std.container into a package. #2174

damianday · 2014-05-16T20:55:20Z

Each container has been put into its own module.
Unit tests have been moved to the appropriate places.
Makefiles have been updated.

I've tried to keep the commits small to make reviewing simpler.

monarchdodra · 2014-05-17T21:04:43Z

Ah, it now compiles. Nice. But I think you lost the commit where you actually got rid of container.d ? I'm seeing 6,754 additions and 19 removals.

dnadlinger · 2014-05-18T01:56:18Z

I applaud every effort to make commits easier to digest, this generally community has much to learn in that respect. However, in this case, all the "duplicate" commits actually make it harder to see what is going on where to me. Maybe you could merge all the commits that just do the same operation to different files together?

A rule of thumb I found helpful: If you can't (be bothered to) write a good commit message for your commit, then it is either too large or too small.

damianday · 2014-05-19T20:50:00Z

@monarchdodra It now removes container.d plenty more removals now ;)

@klickverbot I've had a go at merging the similar commits. The commit history is still a bit messy.

monarchdodra · 2014-05-19T21:08:58Z

@monarchdodra It now removes container.d plenty more removals now ;)

I think you need to rebase over head first, or the merge will fail.

@klickverbot I've had a go at merging the similar commits. The commit history is still a bit messy.

I agree the history could be improved, but I don't think it's a problem during the review proper. I think it's fine if you just squash at the end.

monarchdodra · 2014-05-22T07:58:43Z

LGTM overall. I do have two comments though:

totalcontainer: This doesn't actualy contain anything for the end user. Heck, it doesn't actually contain anything other than (text-only) documentation. While it still might make sense to compile it for the unittesters, I think we can ommit it entirely from std/container/package.d includes. There is no point in including dead code.
I think certain functions, such as make (well, that's currently the only function...), should be considered "global container utility", and included as soon as any package is included. As such, I think there should be a std.container.util package (which may or may not be publicly documented), and included by all the other packages individually too.

DmitryOlshansky · 2014-05-22T22:21:48Z

win32.mak

@@ -442,7 +453,13 @@ cov : $(SRC_TO_COMPILE) $(LIB)
 	$(DMD) -cov=54 -unittest -main -run std\stream.d
 	$(DMD) -cov=53 -unittest -main -run std\socket.d
 	$(DMD) -cov=0  -unittest -main -run std\socketstream.d
-	$(DMD) -cov=88 -unittest -main -run std\container.d
+	$(DMD) -cov=50 -unittest -main -run std\container\array.d


How did coverage dropped this much? Practically by definition some modules must be covered better.

damianday · 2014-05-23T08:01:43Z

@monarchdodra Both those changes do make sense to me, so I've pushed them.

@DmitryOlshansky Code coverage is now corrected.

monarchdodra · 2014-05-23T13:31:13Z

std/container/array.d

+
+import core.exception, core.memory, core.stdc.stdlib, core.stdc.string,
+  std.algorithm, std.conv, std.exception, std.range,
+  std.traits, std.typecons, std.container.util;


The std.container.util import should be public? That's what I had in mind anyways.

monarchdodra · 2014-05-26T10:00:27Z

I've found nothing else to add. There are some imports that can probably still be cleaned-up, but that can be saved for a future pull. Just a final squash, and this gets the LGTM from me.

The only thing I'm unsure about is how this packaging business works in terms of documentation...?

monarchdodra · 2014-05-26T10:13:24Z

Pinging @jmdavis for an informal review. I believe you were the one that designed "package.d"? One question I have is that I was under the impression that the symbols inside a package were a part of the encapsulating module. However, this creates a sub-module for each package. EG:

import std.container.array;

void main()
{
    auto arr1 = std.container.Array(); //NOPE
    auto arr2 = std.container.array.Array(); //OK
}

Did we do something wrong here, or was I simply miss-understood?

jmdavis · 2014-05-26T13:23:04Z

@monarchdodra package.d is pretty much a normal module. It's just that it gives you a way to make it so that a package is also a module. The typical thing to do with it is to publicly import the modules in the package inside of it, in which case, if you import the package, you import everything that was publicly imported in package.d.

So, if std/container/package.d publicly imports all of the modules in std/container/, and you import it, then it's as if you imported all of the module directly. But if you import one of the modules directly, then that's all you're importing. So, in your example, std.container.Array won't work anymore than std.array.popFront will work for arrays if import std.range but not std.array even though it publicly imports std.array, and both popFront and std.range.popFront would work. You'd have to import std.container rather than std.container.array, if you wanted to use std.container.Array.

monarchdodra · 2014-05-26T20:13:03Z

Alright. Well, anybody else want to review this? Seems like a big change to pull without a second opinion.

Turn std.container into a package. Delete container.d Remove totalcontainer from package. Create std.container.util.d and reference it from other containers. Correct code coverage for containers. Add containers for unit testing. Make std.container.util public from any module. Move around imports (avoid version(unittest)). Remove irrelevant unittests.

damianday · 2014-05-28T07:35:47Z

Squashed. Let me know if there are any more issues, thanks :)

DmitryOlshansky · 2014-05-28T20:03:08Z

std/container/array.d

+// Test issue 5920
+version(unittest)
+{
+    //@@@BUG4274@@@: This cannot be declared as an inner struct.


The bug is fixed. Why is this workaround here then ?

Probably because no-one took the time to fix this specific unittest ;)

I think we should be reviewing the packaging, rather than -re-reviewing the totality of container.d...

I am reviewing packaging, namely I'm diffing it against my copy of container.d to see if there were any significant changes other then splitting. In git HEAD this was already fixed.

Oh, really? Dang. I didn't realize, sorry. @damianday, could you make sure no work has been lost compared to the latest head?

DmitryOlshansky · 2014-05-28T20:11:15Z

At least for me generated html is strangely showing only std.container without any submodules, more then that it fails to open. Can anyone double check what the docs look like?

DmitryOlshansky · 2014-05-28T20:12:38Z

Lastly I'd note that totalcontainer.d definitely doesn't deserve its own module being an empty skeleton for documentation purposes.

damianday · 2014-06-06T10:47:37Z

@monarchdodra All modules now synced with git head, all diffs resolved.

@DmitryOlshansky What to do about totalcontainer.d? Perhaps rename to totalcontainer.doc and remove from makefiles etc?

DmitryOlshansky · 2014-06-06T11:25:02Z

@damianday I'd merge it with package as this is where general documentation on containers goes.

P.S. Anther thing is that you need to squash these commits into one.

damianday · 2014-06-06T12:50:17Z

@DmitryOlshansky merged documentation into package and squashed.

monarchdodra · 2014-06-15T17:34:04Z

Needs rebase, it's DList which was modified, btw. Let's get this rebased and pulled quick, so it doesn't desync again.

damianday · 2014-06-17T16:09:28Z

@monarchdodra re-synced and all green.

monarchdodra · 2014-06-17T19:13:29Z

Is the fact that it's actually re-packaged documented anywhere? I can't see it it.

Anywhoo, looks "good enough to merge". Anybody see anything blocking?

DmitryOlshansky · 2014-06-17T19:15:20Z

It would be cool to see how final dpl docs look like after this, last time I tried building docs locally of this branch it wasn't good. Otherwise LGTM

monarchdodra · 2014-06-25T09:16:38Z

Auto-merge toggled on

Turn std.container into a package.

DmitryOlshansky reviewed May 22, 2014
View reviewed changes

monarchdodra reviewed May 23, 2014
View reviewed changes

DmitryOlshansky reviewed May 28, 2014
View reviewed changes

Update to git head.

82544bd

Merge totalcontainer.d with package.d

b52a338

monarchdodra mentioned this pull request Jun 15, 2014

std.container: RedBlackTree.remove readability enhancement. #2247

Merged

Merge remote-tracking branch 'origin/container_package'

579c4a2

monarchdodra added a commit that referenced this pull request Jun 25, 2014

Merge pull request #2174 from damianday/master

a3fd0d4

Turn std.container into a package.

monarchdodra merged commit a3fd0d4 into dlang:master Jun 25, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn std.container into a package. #2174

Turn std.container into a package. #2174

damianday commented May 16, 2014

monarchdodra commented May 17, 2014

dnadlinger commented May 18, 2014

damianday commented May 19, 2014

monarchdodra commented May 19, 2014

monarchdodra commented May 22, 2014

DmitryOlshansky May 22, 2014

damianday commented May 23, 2014

monarchdodra May 23, 2014

monarchdodra commented May 26, 2014

monarchdodra commented May 26, 2014

jmdavis commented May 26, 2014

monarchdodra commented May 26, 2014

damianday commented May 28, 2014

DmitryOlshansky May 28, 2014

monarchdodra May 28, 2014

DmitryOlshansky May 29, 2014

monarchdodra May 29, 2014

DmitryOlshansky commented May 28, 2014

DmitryOlshansky commented May 28, 2014

damianday commented Jun 6, 2014

DmitryOlshansky commented Jun 6, 2014

damianday commented Jun 6, 2014

monarchdodra commented Jun 15, 2014

damianday commented Jun 17, 2014

monarchdodra commented Jun 17, 2014

DmitryOlshansky commented Jun 17, 2014

monarchdodra commented Jun 25, 2014

Turn std.container into a package. #2174

Turn std.container into a package. #2174

Conversation

damianday commented May 16, 2014

monarchdodra commented May 17, 2014

dnadlinger commented May 18, 2014

damianday commented May 19, 2014

monarchdodra commented May 19, 2014

monarchdodra commented May 22, 2014

DmitryOlshansky May 22, 2014

Choose a reason for hiding this comment

damianday commented May 23, 2014

monarchdodra May 23, 2014

Choose a reason for hiding this comment

monarchdodra commented May 26, 2014

monarchdodra commented May 26, 2014

jmdavis commented May 26, 2014

monarchdodra commented May 26, 2014

damianday commented May 28, 2014

DmitryOlshansky May 28, 2014

Choose a reason for hiding this comment

monarchdodra May 28, 2014

Choose a reason for hiding this comment

DmitryOlshansky May 29, 2014

Choose a reason for hiding this comment

monarchdodra May 29, 2014

Choose a reason for hiding this comment

DmitryOlshansky commented May 28, 2014

DmitryOlshansky commented May 28, 2014

damianday commented Jun 6, 2014

DmitryOlshansky commented Jun 6, 2014

damianday commented Jun 6, 2014

monarchdodra commented Jun 15, 2014

damianday commented Jun 17, 2014

monarchdodra commented Jun 17, 2014

DmitryOlshansky commented Jun 17, 2014

monarchdodra commented Jun 25, 2014