Improvements to RedBlackTree #10

Merged
2 commits merged into from Feb 27, 2011

Projects

None yet

3 participants

Member
jmdavis commented Feb 23, 2011

Here are some improvements to RedBlackTree as discussed in the newsgroup. It should improve the remove situation and deals with most everything listed in http://d.puremagic.com/issues/show_bug.cgi?id=5451 . I also made it a class, since we're planning to change all of the containers to std.container, and making it a class fixes the default construction problem that it had.

Steven should probably have a look at this before it gets merged in, though I have no idea when he'll get around to that, since he hasn't gotten around to figuring out git yet. So, if multiple other folks look at it and think that it's good though, then we should probably just merge it in and let Steven worry about making whatever changes he thinks are appropriate later. Certainly, these changes make RedBlackTree more usable.

@jmdavis jmdavis Improvements to RedBlackTree.
1. It's now a class. The only constructor is now the default
constructor (since templatizing the constructors does not appear to
currently work).

2. It now has a helper function - redBlackTree - for creating it.

3. It now has a length property.

4. It now has a version of remove which takes a Take!Range.

5. It now has removeKey to allow you to remove one or more elements by value from it.
aca1a2d

This should be if (is(typeof(binaryFun!less(T.init, T.init))))

probably version(xxx) unittest would be more idiomatic.

There is a very good reason I did this in dcollections. The reason is because doUnittest is only true if the element type is integral. If the doUnittest enum is not used, then simply instantiating a RedBlackTree!string fails because the unit tests are not valid.

If there are better ways to do this, I'd love to change this. I think the way this is done is rather ugly, but it does work.

Owner

Apparently, version can only be used at the module level, so I'm leaving it as is. For the trick to work where it instantiates the unit tests for each instantiation of the template which meets certain requirements (isIntegral!T in this case), you have to use static if rather than version.

I understand. So the flag facilitiates putting unittests next to the methods they are testing. I guess I'd prefer hoisting them out of the class, but I reckon it's good to have the unittest right there.

For reference, here is some ideas I had when I discovered this issue. I believe for unit tests to be maintainable, they need to be next to the function being tested. Otherwise, nobody is going to go to the end of the class and find the unittest to update. If the "unittest to ddoc example" thing ever gets working, then putting unit tests near the function is a requirement.

http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D.learn&article_id=19733

Owner

I totally agree that it's better to have unit tests next to the functions that they're testing, but it doesn't work as well in templated types. And in the general case, I wouldn't expect it to work with the "unittest to ddoc example" thing, because it's likely to be hard to have a working example in the template like that. You have to pull tricks like RedBlackTree is doing and static if unittest blocks based on the template arguments. And if you have to use version(D_Ddoc) it's even worse, since then you can't put a functioning unittest block next to the function. So, there will be cases where it's just not going to work to use the "unittest to ddoc example" thing no matter how desirable it might be.

Regardless, in the general case, it's definitely more maintainable to put the unit tests next to the code that they test. It's a major irritation for me that I had to put the unit tests for the templated types like the interval types after the type definition instead of next to the functions that they tested, and it definitely makes maintaining that harder. So, any time that we can reasonably put the unit tests next to what they test, it's definitely the way to go.

If you wish. I felt dup was a function, not a property. I know the precedent in arrays is as a property, but dup is not a simple accessor. Requiring the parens makes it seem more like a 'function'.

Owner

I really don't think that dup is a property (just like I don't think that save or ranges is really a property), but since it's a property on arrays, it seems better to just make it match and make it a property.

I think we should do what arrays do instead of gratuitously choosing something different. I'd hate to require the user to remember to add parens for containers but not for arrays. For that reason documented intently c.dup without parens in the table in std.container.

One problem is that each instantiation of RedBlackTree will generate and run another unittest. So for parameterized structs and classes it's best to move unittests outside, unless they depend on the template parameters (which doesn't seem to be the case here).

Owner

I believe that that was Steven's intent. I have no problem with moving them out however. The benefit in testing specifically integral types like it's doing is limited.

It only instantiates and runs another unit test if the element type is integral. This is the whole purpose of doUnittest.

I highly recommend to leave things the way they are, I found about 5 dmd bugs while doing unit tests simply because it was extremely easy to add more unit tests for another integer type (I test all 8).

It might be possible to do more types, but I'd rather figure out how to change the current unit tests so they work with the new type also than move the unit tests outside.

Owner

The problem is that then you can't test other comparison functions very well. And honestly, I would consider integral types simple enough that testing all of them is of limited benefit. At most, you find some errors related to narrowing conversions - at least as far as finding bugs in RedBlackTree goes. Maybe there are dmd bugs that it would help find. I have no idea what types of bugs you found before, so I can't really comment on that. But assuming that dmd works correctly, I see limited benefit.

there were a few bugs in containers that I found in dcollections that didn't show up until I was using shorts and ushorts, and the ones I found in dmd were related to AA's with keys smaller than integers (would not have been found here, AA's aren't used)

You should be able to add unit tests for other types. For example, you could also test strings. What needs to be done is to alter the create function so it takes ints and use std.conv.to to convert to Elem. You'd also need to change all casts to to!Elem.

You can always add unit tests outside the class if you want to test other types. But to be able to test comprehensive unit tests across a type by just instantiating the class is very slick I think.

Owner

I agree that declaring unittest blocks inside of templatized types can be very useful, but I've generally found that too many tests don't quite work that way, so I rarely do it. Using static if or a version based on static if as you've done does ameloriate the problem, but it doesn't entirely fix it.

I'm pretty sure you can do here without precomputing len. Use the value of "i" in the loop below at the termination of the loop. Of course you'd need to hoist "i"'s definition outside the for-init-statement.

Would be great to avoid duplication here by e.g. forwarding from the first function to the second. To force an array into a non-array range, use e.g. filter!"true"(elems).

Owner

Ah. There we go. The algorithms that I thought of trying to turn the array into another range type all ended up keeping it as an array.

Oh wait, here you convert to array anyway. So you may decide to forward the other way around.

Owner

Good point.

I was about to comment that I don't like how array allocates here, but I realize why you did it. I can't say I would have made the same choice, but I find no fault with it.

Note, you can specialize removeKey for RedBlackTrees that do not allow duplicates (there will be only one element to remove, no need to use equalRange).

Actually, I question that you remove all elements with the same key, what if you only wanted to remove one of them?

In dcollections, I simply ignored the possibility, and if someone wished to add or remove data to/from itself, I only check the direct cases.

Nice work. A few nits.

Owner

Yeah. Some good points there, though some of them were there in the original code and not my doing (though they're still worth looking at).

Understood. I'm thinking since you're touching the file anyway it's worth making some opportunistic improvements too.

Member

Probably can remove the comment about Bug 2810, I think that was just fixed. However, I haven't tested yet...

Why change this from the ctor that takes elements to using the default ctor and stableInsert?
Not that it's a big deal, it just seems like an odd change.

Owner

The problem is the inability to have properly templatized class constructors. So, I just simplified it so that you have just the default constructor and added the redBlackTree helper function to essentially take the place of the constructor (with the added benefit of not necessarily having to give the element type). I suppose that we could have this(E[] elems...), but the templatized version that takes implicit conversions into account and whatnot won't work. But it just seems simpler to me to push off construction to redBlackTree in all cases where you don't want default construction rather than having it work with the constructor in some cases but be forced to use redBlackTree in others.

Yes, I wrote this before I saw you disabled the template ctors.

this(E[] elems...) should do implicit conversions without issue, as long as you are passing elements. What you won't be able to do is instantiate with a range type.

However, it should be fully backwards compatible to later remove the this(E[] elems...) and add the template version, so I'd rather see it added now. To have new RedBlackTree!int(1,2,3,4,5) not work seems unintuitive.

Member

It is probably not necessary to add length checks to so many unit tests.

Member

BTW, I like the scope(success) usage to alter length, I'm going to steal that idea for dcollections too ;)

I think this can be done simpler:

auto b = r.original._begin;
while(!r.empty) r.popFront(); // move take range to its last element
auto e = r.original._begin;
while(b != e) {b = b.remove(_end); --_length;}
return Range(e, _end);

(BTW, a lot of the above has leading underscores, but github seems to think that's markup for italics, sorry)

Also, I would expect the removal of an empty take range to return the range that starts from wherever the take range started. The code above does that, your code does not.

for example:

auto ts = create(1,2,3,4,5);
auto r = ts[];
r.popFront();
auto r2 = ts.remove(take(r, 0));
assert(std.algorithm.equal(r2, [2,3,4,5]));

Owner

I'll adjust it then.

OK, I see now why you stopped using the ctor above.

However, as an intermediate step, let's just create a ctor like:

this(Elems[] elems...)

instead of having to use create (which wasn't a template anyways). When template ctors become possible, changing this to a template should be a completely backwards compatible change.

Member
jmdavis commented Feb 24, 2011

@schveiguy I could probably remove length from some of the unit tests, but given all of the various places that length has to be changed, it seemed a lot safer to just be thorough about it.

Can the above two be merged into:

auto redBlackTree(alias less, bool allowDuplicates=false, E)(E[] elems...)

or does that not work?

Owner

The reason that I have so many versions or redBlackTree is that you can give any combination of the template parameters and let the others be their default values rather than being forced to give some of the parameters, because the one you want to set is later in the list. It's possible that I declared one more than is needed, but I'd have to mess around with it to be sure.

Owner

Okay. It turns out that they can't be merge. You get conflicts if you try. On top of that, there's some weird bug where the last one doesn't work, because RedBlackTree barfs on less with an error like this:

std/container.d(4136): Error: this for _end needs to be type RedBlackTree not type std.container.RedBlackTree!(int,"a > b",true).RedBlackTree

However, passing binaryFun!less to the template instantiation of RedBlackTree fixes the problem.

OK, having the extra templates doesn't hurt. It really was a trivial nitpick.

Thanks for trying though.

Member

Overall: looks good, need to fix that remove+Take function (see comment about what should be returned when removing an empty range).

@jmdavis jmdavis Changes to RedBlackTree per suggestions on pull request #10 of D-Prog…
…ramming-Language/Phobos.

I think that I took all of the suggestions into account, so it should be
improved per those suggestions. I also beefed up the unit tests a bit in
order to take different less and allowDuplicate values into account
(previously, it was only testing the defaults for those template
parameters).
858865a
Member
jmdavis commented Feb 24, 2011

I've made the changes as suggested and improved the unit tests a bit.

I like how you are expanding the unit tests to test the duplicate version and altering the ordering.

I think this should be:

else static if(less == "a > b")

because there is a possibility some other instantiation would be created with an integral type, and the unittest shouldn't assume the ordering.

Nice, I like this test jig. Also stealing for dcollections ;)

I think this commit looks good enough. The "else static if" comment isn't critical to the patch, so if we want to accept this as is, I approve.

Owner

I tried to merge this, by following the instructions. However, unittests fail:

std/datetime.d(28538): Error: undefined identifier tzname, did you mean function name?

What's going on?

Member
jmdavis commented Feb 27, 2011

Update druntime. The declaration of tzname got moved to drutime where it belongs.

Owner

Done.

@kyllingstad kyllingstad pushed a commit to kyllingstad/phobos that referenced this pull request Apr 26, 2011
@jmdavis @andralex jmdavis + andralex Changes to RedBlackTree per suggestions on pull request #10 of D-Prog…
…ramming-Language/Phobos.

I think that I took all of the suggestions into account, so it should be
improved per those suggestions. I also beefed up the unit tests a bit in
order to take different less and allowDuplicate values into account
(previously, it was only testing the defaults for those template
parameters).
c6874cb
@burner burner added a commit to burner/phobos that referenced this pull request Jul 14, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
99e1208
@burner burner added a commit to burner/phobos that referenced this pull request Jul 15, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
5367827
@burner burner added a commit to burner/phobos that referenced this pull request Jul 16, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
179e57c
@burner burner added a commit to burner/phobos that referenced this pull request Jul 24, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
9cfaa7d
@burner burner added a commit to burner/phobos that referenced this pull request Aug 8, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
3c7da8d
@burner burner added a commit to burner/phobos that referenced this pull request Aug 18, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
95f9901
@burner burner added a commit to burner/phobos that referenced this pull request Aug 19, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
6ef364a
@burner burner added a commit to burner/phobos that referenced this pull request Aug 26, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
6cfdf48
@burner burner added a commit to burner/phobos that referenced this pull request Sep 8, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
c678640
@burner burner added a commit to burner/phobos that referenced this pull request Sep 15, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
7c64303
@burner burner added a commit to burner/phobos that referenced this pull request Sep 15, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
d24f031
@burner burner added a commit to burner/phobos that referenced this pull request Sep 25, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
ac9c8b3
@burner burner added a commit to burner/phobos that referenced this pull request Oct 2, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
d6d46d4
@burner burner added a commit to burner/phobos that referenced this pull request Oct 3, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
0c093fe
@burner burner added a commit to burner/phobos that referenced this pull request Oct 10, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
f0aac4b
@burner burner added a commit to burner/phobos that referenced this pull request Oct 11, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
0c80d7d
@burner burner added a commit to burner/phobos that referenced this pull request Oct 11, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
2d7821e
@burner burner added a commit to burner/phobos that referenced this pull request Nov 9, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
8bdd5a7
@burner burner added a commit to burner/phobos that referenced this pull request Nov 12, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
d12bbd2
@burner burner added a commit to burner/phobos that referenced this pull request Nov 19, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
d393a37
@burner burner added a commit to burner/phobos that referenced this pull request Nov 25, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
1c73233
@burner burner added a commit to burner/phobos that referenced this pull request Dec 10, 2014
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
9de481a
@burner burner added a commit to burner/phobos that referenced this pull request Dec 25, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
0498998
@burner burner added a commit to burner/phobos that referenced this pull request Dec 26, 2014
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
7b82a45
@burner burner added a commit to burner/phobos that referenced this pull request Jan 3, 2015
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
836fab7
@burner burner added a commit to burner/phobos that referenced this pull request Jan 3, 2015
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
d58dec4
@burner burner added a commit to burner/phobos that referenced this pull request Jan 18, 2015
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
1d6b84e
@burner burner added a commit to burner/phobos that referenced this pull request Jan 22, 2015
@burner @burner burner + burner thread safe and concur works comments for free standing log functions
fixes #10
4e51092
@burner burner added a commit to burner/phobos that referenced this pull request Jan 25, 2015
@burner burner thread safe and concur works comments for free standing log functions
fixes #10
f648be2
@burner burner added a commit to burner/phobos that referenced this pull request Jan 25, 2015
@burner @burner burner + burner std.logger
some more docu fixes

fatel -> fatal

changed the way of calling

some rework

some better comments and win32.mak fix

more documentation

trying to figure out why win32 fails

test before you commit

another try to work around windows files and processes

maybe this time

maybe this merges

update 1 mostly soenke

MultiLogger and Logger have names

more docu

unittest fix

some better comments and win32.mak fix

Conflicts:
	std/logger.d

antoher doc fix

less code dup and more log functions

docs are now generated

some more fixing

speedup

some threading

forgot some

better docu gen and more testing

two new LogLevel, most functions are now at least @trusted

Tracer functionality

fatal delegate

some phobos style fixes

another bug bites the dust

version disable enhancements

default global LogLevel set to LogLevel.all

logLevel compare bugfix

delete of dead code

tab to whitespace

bugfixes, reduandency removal, and docu

multilogger was logging to all it childs without checking there LogLevel in
relation to the LogLevel of the LoggerPayload.

Some constructors where redundant.

more examples and more documentation

some fixes

NullLogger and moduleName

wrong doc

I splitted the multi logger implementations out

loglevelF to loglevelf in order to make phobos style think writefln

document building

win makefile fixes

some optimizations thanks to @elendel-
some whitespace

some updates from the github logger project

* stdio- and filelogger now use a templatelogger base to reduce code

makefile fixes

makefile

fixed the unittest

made sure the stdiologger is set up at the end

some comment fixes

finding the filelogger segfault

closed another file

a lookup fix

darwin unittest fail output

more diagnostics

* more documentation for the templatelogger and multilogger
* it breaks the log function api
 * log and logf now behave as their write and writef counterparts
 * for logging with an explicit LogLevel call logl loglf
 * for logging with an explicit condition call logc logcf
 * for logging with an explicit LogLevel and explicit condition call
 * loglc loglcf
 * the log function with an explicit LogLevel like info, warning, ...
 * can be extended into c, f or cf and therefore require a condition
 * and/or are printf functions

something is still interesting

rebase and lazy Args

saver file handling

whitespace

some updates

tracer is gone and more doc updates

codegen rework to allow log function comments

thread safe and concur works comments for free standing log functions

fixes #10

* free log function doco
* StdIOLogger prints now threadsafe
* concurrentcy hang fix

more docu

old file

even more doc

typo

* more better doc
* makefile fix

another fix

more unittests nearl 100% everywhere

another test

no more mixins more doc

win64 please pass

fixes from the review and voting

hope this fixes it

more comments

more docs more tests

more docu

more doc and fixes from jacob-carlborg

LogEntry.logger reference

more fixes

fileptr is gone

logger move to experimental/logger

type in win32/64 makefile

win makefile

nogc and threading

makefile fixes

some log calls take line and file as parameter
04aa499
@andralex andralex referenced this pull request in andralex/phobos Feb 1, 2015
@burner @andralex burner + andralex std.logger
some more docu fixes

fatel -> fatal

changed the way of calling

some rework

some better comments and win32.mak fix

more documentation

trying to figure out why win32 fails

test before you commit

another try to work around windows files and processes

maybe this time

maybe this merges

update 1 mostly soenke

MultiLogger and Logger have names

more docu

unittest fix

some better comments and win32.mak fix

Conflicts:
	std/logger.d

antoher doc fix

less code dup and more log functions

docs are now generated

some more fixing

speedup

some threading

forgot some

better docu gen and more testing

two new LogLevel, most functions are now at least @trusted

Tracer functionality

fatal delegate

some phobos style fixes

another bug bites the dust

version disable enhancements

default global LogLevel set to LogLevel.all

logLevel compare bugfix

delete of dead code

tab to whitespace

bugfixes, reduandency removal, and docu

multilogger was logging to all it childs without checking there LogLevel in
relation to the LogLevel of the LoggerPayload.

Some constructors where redundant.

more examples and more documentation

some fixes

NullLogger and moduleName

wrong doc

I splitted the multi logger implementations out

loglevelF to loglevelf in order to make phobos style think writefln

document building

win makefile fixes

some optimizations thanks to @elendel-
some whitespace

some updates from the github logger project

* stdio- and filelogger now use a templatelogger base to reduce code

makefile fixes

makefile

fixed the unittest

made sure the stdiologger is set up at the end

some comment fixes

finding the filelogger segfault

closed another file

a lookup fix

darwin unittest fail output

more diagnostics

* more documentation for the templatelogger and multilogger
* it breaks the log function api
 * log and logf now behave as their write and writef counterparts
 * for logging with an explicit LogLevel call logl loglf
 * for logging with an explicit condition call logc logcf
 * for logging with an explicit LogLevel and explicit condition call
 * loglc loglcf
 * the log function with an explicit LogLevel like info, warning, ...
 * can be extended into c, f or cf and therefore require a condition
 * and/or are printf functions

something is still interesting

rebase and lazy Args

saver file handling

whitespace

some updates

tracer is gone and more doc updates

codegen rework to allow log function comments

thread safe and concur works comments for free standing log functions

fixes #10

* free log function doco
* StdIOLogger prints now threadsafe
* concurrentcy hang fix

more docu

old file

even more doc

typo

* more better doc
* makefile fix

another fix

more unittests nearl 100% everywhere

another test

no more mixins more doc

win64 please pass

fixes from the review and voting

hope this fixes it

more comments

more docs more tests

more docu

more doc and fixes from jacob-carlborg

LogEntry.logger reference

more fixes

fileptr is gone

logger move to experimental/logger

type in win32/64 makefile

win makefile

nogc and threading

makefile fixes

some log calls take line and file as parameter
acb31a3
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment