Deprecated C++ features actually removed from compiler

[elelel]

Dlib uses some very old C++ features that are deprecated, like auto_ptr. Usually this led to compiler warnings that these features should not be used and this was 'fixed' just by quelling them: https://github.com/davisking/dlib/blob/master/dlib/smart_pointers/shared_ptr.h#L13 Now under some scenarios these deprecated features became unavailable at all. For example, in Microsoft Studio 2017 when compiling with -std:c++latest option auto_ptr is simply non-existent identifier. This means DLib's header files can't be used in projects that use C++17 libraries/code unless it is compiled as a separate module wrapped into a compatible interface. Though the C++17 is not a standard yet, and neither it is known to me if Microsoft plans to get rid of auto_ptr for good, I think it still makes since to let people at least prototype in what might become C++17. I'd suggest that any features marked as deprecated in the standard should be used only when required (e.g. if an old compiler is detected) and not by default. To solve this problem for at least in my own case I would like to replace the dlib's shared_ptr with stdlib's one. Is it safe to assume that I can just get away with replacing the implementation with standard one, or are there any behavioural difference between the two?

[davisking]

There aren't any important differences between the two. So if you want to make a pull request that just completely replaces dlib's shared_ptr with the std one that's fine with me. You should probably put a using statement in dlib's namespace though so that dlib::shared_ptr still works for old code, but other than that I don't think there are any backwards compatibility worries.

[elelel]

I looked through the smart pointers code and noticed that a conversion would not be that straightforward:

• dlib::shared_ptr interface is not fully compatible with std::shared_ptr. In particular, it exposes uinque() which is marked deprecated in C++17 for the reason it can't be exact in multithreaded environments. I don't know if they will also delete it as they did with auto_ptr
• auto_ptr is also used in shared_ptr_thread_safe which has no equivalent counterpart int stdlib. Perhaps switching auto_ptr to something like unique_ptr in the implementation makes sense, but I'm not sure about the current logic and interchangeability

[davisking]

I know. But I suspect mightily that no one uses those features. It's hard to be sure. But even so, everyone should switch to the std versions of these things anyway.

The very few people who might be using them can just grab a copy of these objects from an older version of dlib if they really want to keep using them in the future. WIth that in mind, it's probably best to avoid putting any shared_ptr name in the dlib namespace.

[elelel]

In this case I think I can make a PR replacing all dlib::shared_ptr with std::shared_ptr, but what about shared_ptr_thread_safe? Leaving it as is still leaves the library uncompilable under currently proposed C++17. Even if people don't use the header file, they won't be able to compile it easily as a source-based dependency.

[davisking]

I would switch them all to shared_ptr, including any use of shared_ptr_thread_safe. I think the most straightforward thing to do is to switch everything in dlib to use std::shared_ptr and to make sure no files #include anything from dlib/smart_pointers. Then leave the code in dlib/smart_pointers untouched. People who want to use it can use it, but anyone who isn't explicitly #including it in their own code won't be bothered by it.

[elelel]

I think I will have to refactor smart_pointers, because there're "good" smart pointers, like scoped_ptr, which have to be includable in the modern version. Old smart pointers will be factored out into something like dlib/legacy_smart_pointers

[davisking]

Why would someone use scoped_ptr when there is unique_ptr though?

[elelel]

The library itself uses scoped_ptr quite actively. Of course, this can be replaced too, but I would rather replace each type of pointer incrementally. My plan was to replace first weak_ptr/shared_ptr, get the whole thing compilable, submit it as a separate PR, then to get rid of thread_safe version, and then proceed to scoped_ptr, etc. In case there's some bug because of the differences I did not take in account when replacing, we can always easily roll back the 2/3 of the changes while leaving the most obvious 1/3 that is incidentally also the part required to make the library usable under C++17. In other words, scoped_ptr is surely needs to be replaced as well, but the gains are zero for the compilation under C++17 aim, while the risks are always non-zero as with any change.

[davisking]

That sounds fine. I would still say to leave the existing smart pointer files alone so that existing users who are #including them aren't bothered. So when removing an old smart pointer from use in dlib you should be able to just stop #including it in other dlib files and replace usages with the equivalent std:: smart pointer.

[elelel]

I would switch them all to shared_ptr, including any use of shared_ptr_thread_safe. I think the most straightforward thing to do is to switch everything in dlib to use std::shared_ptr and to make sure no files #include anything from dlib/smart_pointers.

I checked shared_ptr_thread_safe usage and it looks like it is involved in the threading part of dlib. Are you sure you want to replace them blindly? It is relied on in thread_pool_extension.h and at a first glance it's not something I would replace with a threads-ignorant version. The optimal variant would be to get rid of the antiquated thread pool class itself, but I'm not familiar enough with DLib's architecture to do that. The class is used by quite a lot of clients: logger, sockets, gui_widgets, memory_manager, image_processing, some tests and maybe others. Please confirm that you're ok with replacing shared_ptr_thread_safe in thread_pool_implementation class with std::shared_ptr without any concurrency checks. In this case I'll submit a PR with that.

[elelel]

I examined the code and it seems that thread pool class doesn't expose the impl shared_ptr_thread_safe pointer to any threads referentially. Also it looks to me that shared_ptr_thread_safe guarantees are not stronger that these of std::shared_ptr, so we are safe to just replace it. I still would like to get your confirmation to proceed, though

[davisking]

Oh it's fine :). dlib::shared_ptr_thread_safe is meant to have the same thread safety guarantees as std::shared_ptr. I always intended to replace it with std::shared_ptr when std::shared_ptr eventually became widely supported.

I added the thing about 9 years ago and every time I used it I thought about how nice it would be to be able to replace it with std::shared_ptr. It's finally time to replace it with std::shared_ptr. So feel free to make the PR :)

[elelel]

When converting scoped_ptr, it iturned out that it's interface is not fully compatible with std::unique_ptr; smart_pointers.cpp test uses evaluation of scoped_ptr as bool which is not something unique_ptr supports. Do you want me to treat the scoped_ptr exported to old clients as we did with shared_ptr (i.e. export the old version in smart_pointers.h)?

[davisking]

Huh, yeah, I didn't realize that. That's surprising that the bool conversion is missing. Don't worry about it then. We can just leave it alone. It's not like std::shared_ptr which does have a lot of really nice features.

Maybe a reasonable thing is to upgrade scoped_ptr with move constructors and possibly an implicit conversion to unique_ptr.

[elelel]

According to the spec the operator bool should be included: http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool ...but I must be doing something wrong: tester.h:20:67: error: cannot convert ‘dlib::scoped_ptr<{anonymous}::base> {aka std::unique_ptr<{anonymous}::base, std::default_delete<{anonymous}::base> >}’ to ‘bool’ for argument ‘1’ to ‘void test::check_test(bool, long int, const char, const char)’

[elelel]

I think the problem was again that something's got cached and not recompiled. Just cleaned the whole thing and recompiled and it worked. Current CMake scripts seem to prevent detection of source changes somehow. Maybe they should be refactored to a newer add_subdirectory() style (instead of including the whole dlib directly).

[davisking]

There shouldn't be any caching or anything. The CMake scripts do already use add_subdirectory(). Maybe it's a bug in the version of cmake you are using.

[elelel]

Right, besides it shouldn't affect the .h files, because they're always recompiled anyway. I don't why it happens, I use CMake extensively and that's the first time I see this with my current install. I found why operator bool didn't work: it is declared with explicit specifier in STL, and the dlib function that is used in test macro requires it's argument be passed as copy by value. I didn't touch the function, but instead rewrote the calls to macros with explicit bool construction, and that fixed it. The function should be reworked though to be more generic. I'm running the final test and will submit PR afterwards. The template alias works, but be aware that there almost surely be people who will manage to break it, and complain. Template aliases can never be 1-to-1 maps to templates, I think, because of reference deduction intricacies.