Implement UniquePtr #1
Conversation
|
My review of Teuchos::UniquePtr class ... So I have finished my initial review of the Teuchos::UniquePtr class. I found a few little issues and made a few small commits that I pushed the uniqueptr branch in my forked github repo: Please review those commits and the commit messages and the commits in my detailed review below. The only major remaining todos that I can see for this UniquePtr class are:
Otherwise, this looks very nice. It is interesting to see how some of the new C++11 traits classes are being used. That makes programming code like this so much easier. I just really wish we did not have to write memory management code of our own like this anymore. I am going to try to look at writing an updated white paper for the current status of the Teuchos MM classes in relation to the C++11/14 standard libraries. 1) Get the code and inspect the changes Ondrej has created a github clone of the Trilinos public git repo at: The pull request containing the UniquePtr code is: I added the remote and got the branch: Since this is the filtered public repo, it has not commits in common with the main Trilinos git repo :-( But you can still do this in the same git repo. Looking at the changed files compared to ondrej/master: The only change to the file Teuchos_toString.hpp is: Very good. Just as you would expect. It would seem that nullptr is a special keyword in C++ and not like an enum really. But the type std::nullptr_t can be used to pass it. Compiler magic. Very interesting. The only other change it to the file teuchos/core/test/MemoryManagement/CMakeLists.txt which is just to add the unit tests in UniquePtr_UnitTests.cpp. Very good. Everything else is new code. It looks like all of the unit tests in UniquePtr_UnitTests.cpp are protected by: so they should be excluded when C++11 is not enabled. 2) Configure, build, and test: This gave the test results: Running the unit tests for UniquePtr manually: It looks like there is some problems with printing in a few of these tests. I will fix that. ... I was able to fix that by using [&] with the lambda functions and I did so in the commit: 3) Run coverage build and submit to Trilinos dashboard: Now to do a coverage build and submit to Trilinos CDash: The produced the coverage results at: It shows 100% coverage for UniquePtr.hpp: http://testing.sandia.gov/cdash/viewCoverageFile.php?buildid=2095398&fileid=76763 However, the coverage output does not show executable lines for functions like: So is this function tested or not? To test for that, I added TEUCHOS_TEST_FOR_EXCEPT(true) and built and ran the unit tests ... Okay, so it is being tested. That means that lcov/gcov on my machine is not working correctly. My guess is that this is because I am using a custom installed version of GCC 4.8.3 and it is using lcov/gcov for the old gcc 4.4.7 compiler on this machine. So much for coverage results. I will just look at the code manually and match it with with the unit tests. I will then add throws or break the code to assert the unit tests are working correctly. 4) Look at the code itself: Now I will look at the code in Teuchos::UniquePtr.hpp carefully, line-by-line. There is not much code, so this should not take very long. The first thing that I note is that it is lacking any Doxygen documentation. It will need at least the most basic Doxygen comments or the class nor any of its members will show up in the Doxygen documenation at all. I am noticing that there is a mixture of 2 spaces and 4 spaces for indenting In C++, the convention is to name typedefs as _t. Because of this, I think it would be a good idea to change the typedef Teuchos::UniquePtr::pointer to Teuchos::UniquePtr::pointer_t. Also, there are some C-style casts being used in the code. We don't want C-style casts as they allow very bad things to happen silently. Note that when is a built-in type, then () is a C-style cast equivalent to (). To avoid the pitfalls of C-style casts, in C++11 (thank goodness) we can use {}. Prior to C++11, we had to write our own safe implicit cast function like Teuchos::as() (see the Thyra coding guidelines document). The implementation of the private data of UniquePtr uses an std::tuple<pointer_t, Deleter>. From our prior emails, this is used to save on memory of the Deleter object when it has no data. However, using raw integers to access these data-members (e.g. std::get<0>(t_) to mean the pointer data-member) does not make for very readable code. Therefore, to clean this up, I created private inline member helper functions. I have done this with the inline private members thisPtr(). It is shorter and more descriptive and should have zero overhead. The Deleter object is only accessed in two functions so I did not bother adding helpers for it. If the Deleter object was accessed in more functions, I would create a helper function for it too (but it is more complicated because it could be a reference type as well?). I have not read every line of the unit tests but they look reasonable. The tests that are called by the template function test_unique_ptr_interface() don't provide very good error reporting. When I was playing around adding exceptions to make sure that certain functions were being called, it was hard to see where in that long function the exception was being thrown from. But I guess if a unit test failed, it would have printed the line number so it would not have been a bid deal to track down. It is just uncaught exceptions that are the problem debugging. |
|
Thanks @bartlettroscoe for the thorough review. I am going to squash my 83 commits into a few nice logical commits, and then rebase your commits (which I agree with) on top. And then push in more commits fixing the things you raised, mainly documentation. I will keep you updated. P.S. The |
|
Hi, sorry to resurrect such an old thread, but what happened to this PR? I saw its link on the SymEngine website, so I came here to see what happened to it. I am planning to do a GSoC project with SymEngine and implement the polynomial classes. Since this seems mostly complete (with just the documentation remaining), I could write the documentation as a part of my GSoC project and we could try to get this feature into Trilinos. |
|
@wermos I didn't have time to finish it. I still think it would be nice to have, but given that nobody asked for it in the 7 years since I started it, it seems that maybe the need is not that high. For performance, we want to use custom versions of these pointer classes as we do in SymEngine. And make them safe in Debug mode. But I'll admit I now use regular raw pointers as well as references in C++, essentially I use the language as it was intended. It is not 100% safe, but in the past 10 years that I've been using C++ almost every day, I never had to debug any difficult memory issue, so to me it means it is not that big of a deal in practice. That being said, I think somebody should write a C++ compiler plugin that restricts the C++ features to a subset and checks all raw pointers and references at runtime to ensure they are not dangling. That way one can use the simple direct syntax and it would still be 100% safe. |
This is a work in progress. I will keep pushing more patches as I implement things, and at the very end I will rebase and create a few very nice patches.
TODO:
std::unique_ptr, i.e. add a template parameter, but unlikestd::unique_ptrspecialize the template for the most common default case for efficiency --std::unique_ptruses a tuple to store both the pointer and the deallocator)sizeofUniquePtr is the same as raw pointer, as long as the deallocator doesn't have any member variablesRCP(appropriately adapted toUniquePtr)std::unique_ptr, or as close as possibleUniquePtr::operator=()should release the old object first, sostd::swapis not the way to go (write a test first that fails, then fix it)UniquePtr::swap()swaps the deleter as well