erikerlandson / st_tree

A fast and flexible c++ template class for tree data structures
https://github.com/erikerlandson/st_tree/wiki
Apache License 2.0
95 stars 20 forks source link

Added emplace functions #22

Closed emmenlau closed 4 years ago

emmenlau commented 4 years ago

This PR adds emplacement operators to construct leaves / nodes. There is a new emplace() that mimics insert() but constructs the object in-place, and emplace_back() that mimics push_back() but in-place.

Please note that C++11 is required for the emplace methods due to the use of variadic templates. I hope this is not of concern?

erikerlandson commented 4 years ago

Thanks @emmenlau, this looks useful! I have a couple questions:

  1. Does -std=c++11 require exactly c++11, or is it "c++ >= 11" ?
  2. can you please add unit tests for emplace ?
emmenlau commented 4 years ago

The option -std=c++11 enables at least c++ == 11 in the compiler. With Visual Studio this will not change the default, because modern extensions are always enabled. For gcc and clang, it will set exactly c++ 11, unless the user would override it. But, there should be no harm, because the flag is not passed transitively to downstream projects, and (since st_tree is header-only) the flag is used only for tests and examples. Therefore the flag should not make any difference for users of st_tree. However users of st_tree would need to enable c++11 or newer to use the emplace options.

emmenlau commented 4 years ago

About the unit tests, I was already concerned you may ask. We don't have super much spare resources right now so it may not happen in the next month or two.

erikerlandson commented 4 years ago

True it is header only, however it changes the nature of what compilers can use the project. C++11 is approaching a decade old, so I'm not extremely concerned about requiring that :D Still, it would technically require me to discontinue my "c++98" claims.

It's actually been several years since I've programmed in C++, so I'm trying to be careful about understanding candidate changes. Is emplace useful for performance reasons, as in only calling the constructor at insertion, instead of creating an object and then copying it into the tree?

I'd prefer to not merge any changes that have no unit tests: I'd be satisfied with relatively simple tests: just one or two calls to emplace for each storage type to "smoke test" that it works. It does not seem like a complicated or subtle method. A very simple class with simple constructor would suffice, I think. If you can provide an example or two, I might try writing unit tests for myself, but my c++ is rusty :joy:

emmenlau commented 4 years ago

Hi @erikerlandson , yes I can relate to everything you say. If you are concerned about the required c++11 standard or about the missing tests I think its completely fine not to merge the changes. We just wanted to make them available because we like and use them.

The emplace methods that we added are mostly useful for convenience because they construct the object in-place, so they make the adding of objects a tiny bit simpler. Its really just a convenience thing and saves one copy, but I doubt that the performance is any better. These kind of emplace methods is also becoming more standard, and many stl-containers have them now. It may be nice if st_tree has them too, but again, mostly because it makes st_tree look even more like a standard container.

Internally we use the same mechanism as push_back is using, that is why we did not add tests.

erikerlandson commented 4 years ago

@emmenlau this seems like a good idea and I would definitely like to merge it, I just want to get a few basic unit tests in there. Maybe I'll fool around with it if I have time over the holidays :smiley:

emmenlau commented 4 years ago

We've just added a test for the emplace methods, and it seems to work well. It also works well for us :-)

erikerlandson commented 4 years ago

LGTM @emmenlau, merging