[scope] A few questions

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2023-11-30 14:54:50

Hi,

I have a few questions regarding the scope library currently being 
reviewed:

* scope_exit is not default constructible. But it can be released and is 
movable, so a default constructed scope_exit state makes sense (this is 
the same as a moved-from scope_exit). I find this a rather strange 
design choice. What is the rationale behind this?

There's even a section on the documentation on how to circumvent this:
https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html#scope.scope_guards.runtime_defined

using cleanup_func_t = std::function< void() >;
// Create an inactive scope guard first, since the cleanup function is 
not set yet
boost::scope::scope_exit< cleanup_func_t > cleanup(cleanup_func_t(), 
false);

Why can't i just write?

boost::scope::scope_exit< std::function< void() > > cleanup;

* the documentation says little about memory allocation. I finally found 
the answer i was looking for in the boost.ScopeExit comparison: 
scope_xxx objects do not do any heap allocations themselves. Is that a 
guarantee of the library? Such a guarantee is needed in some code i work 
with, i need to know if it is a feature of the library that i can rely 
upon, and not just an optimization that could go away in a further 
release.

* unique_fd is a resource wrapper for files. But on all major systems, 
closing a file can fail and return an error. It may well be that the 
file is not completly written correctly, and this is reported not in the 
write call, but in the final close call. unique_fd silently drops the 
return value of close. I'm not sure we should encourage this behaviour.

Another issue raises when we look at the example code:

void write_string(std::string const& filename, std::string const& str)
{
     // Create a unique resource for a file
     boost::scope::unique_resource< int, fd_deleter > 
file(open(filename.c_str(), O_CREAT | O_WRONLY));
     if (file.get() < 0)
     {
         int err = errno;
         throw std::system_error(err, std::generic_category(), "Failed to 
open file " + filename);
     }

     // Use it to write data to the file
     const char* p = str.data();
     std::size_t size_left = str.size();
     while (size_left > 0)
     {
         ssize_t written = write(file.get(), p, size_left);
         if (written < 0)
         {
             int err = errno;
             if (err == EINTR)
                 continue;
             throw std::system_error(err, std::generic_category(), 
"Failed to write data");
         }

         p += written;
         size_left -= written;
     }
}

In case of a failure, what we got is an half-baked file, which we know 
nothing about. A proper write_string function should at least try to do 
its cleanup and delete the file in case of error. But that defeats the 
purpose of unique_resource... In my experience, automatically closing 
file handles is not error handling. But it has its use cases, maybe it 
would be better demonstrated by an example like:

bool compare_files(std::string const& filename1, std::string const& 
filename2)
{
     boost::scope::unique_resource< int, fd_deleter > 
file1(open(filename.c_str(), O_CREAT | O_WRONLY));
     if (file1.get() < 0)
     {
         int err = errno;
         throw std::system_error(err, std::generic_category(), "Failed to 
open file " + filename1);
     }
     boost::scope::unique_resource< int, fd_deleter > 
file2(open(filename.c_str(), O_CREAT | O_WRONLY));
     if (file2.get() < 0)
     {
         int err = errno;
         throw std::system_error(err, std::generic_category(), "Failed to 
open file " + filename2);
     }
     // do the comparison
}

Regards,

Julien