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

Turn std.container into a package. #2174

Merged
merged 4 commits into from
Jun 25, 2014
Merged

Turn std.container into a package. #2174

merged 4 commits into from
Jun 25, 2014

Conversation

damianday
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator

LGTM overall. I do have two comments though:

  1. 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.
  2. 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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@damianday
Copy link
Contributor Author

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

@DmitryOlshansky Code coverage is now corrected.


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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@monarchdodra
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member

jmdavis commented May 26, 2014

@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
Copy link
Collaborator

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
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DmitryOlshansky
Copy link
Member

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
Copy link
Member

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

@damianday
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

@DmitryOlshansky merged documentation into package and squashed.

@monarchdodra
Copy link
Collaborator

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

@damianday
Copy link
Contributor Author

@monarchdodra re-synced and all green.

@monarchdodra
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

Auto-merge toggled on

monarchdodra added a commit that referenced this pull request Jun 25, 2014
Turn std.container into a package.
@monarchdodra monarchdodra merged commit a3fd0d4 into dlang:master Jun 25, 2014
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

Successfully merging this pull request may close these issues.

5 participants