Re: [boost] [filesystem] scoped_file feature request?

Subject: Re: [boost] [filesystem] scoped_file feature request?
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-10-13 07:56:08

Jorge Lodos Vigil wrote:
> Beman Dawes wrote:
>
> > If you guys can come up with a good design for a temporary
> > file feature, implement and test it, and provide at lease a
> > sketch of some docs, I'd be happy to add it to the library.
> > But I need to spend my time on other aspects of the library.
> >
> > One feature I've used a lot is the ability to not actually
> > delete the file (or directory - I often need temporary
> > directories). When I'm tracking down a bug it is often
> > helpful to retain the file so it can be inspected. That also
> > implies knowing the path to the file.
>
> Is the following "good design"?
>
> // The same as unique_path except that an empty file is
> created.
> path unique_file(const path& model="%%%%-%%%%-%%%%-%%%%")
>    throws(...);
> path unique_file(const path& model, system::error_code& ec);
> path unique_file(const path& model="%%%%-%%%%-%%%%-%%%%",
> const path& parent_path) throws(...);
> path unique_file(const path& model, const path& parent_path,
>    system::error_code& ec);
>
> // Unique directories are created
> path unique_directory(const path& model="%%%%-%%%%-%%%%-%%%%")
>    throws(...);
> path unique_directory(const path& model,
>    system::error_code& ec);
> path unique_directory(const path& model="%%%%-%%%%-%%%%-%%%%",
>    const path& parent_path) throws(...);
> path unique_directory(const path& model,
>    const path& parent_path, system::error_code& ec);

I like the names "unique_file" and "unique_directory".  unique_path() returns a path which may or may not point to something "real", while unique_file() and unique_directory() return "real" things: a file or a directory.

I don't understand the second overload in each case, though I realize they follow the pattern set by unique_path().  I would have expected the following instead:

   unique_file(system::error_code &,
      path const & = "%%%%-%%%%-%%%%-%%%%");

That permits using an error code and the default pattern.

Similarly, the third overload in each case is faulty.  They are even erroneous, since defaulted parameters must be last.  Here's what I'd prefer:

   path
   unique_file(path const & = "%%%%-%%%%-%%%%-%%%%");

   path
   unique_file(system::error_code &,
      path const & = "%%%%-%%%%-%%%%-%%%%");

   path
   unique_file(path const & _parent, path const & _pattern);

   path
   unique_file(system::error_code &, path const & _parent,
      path const & _pattern);

This allows the pattern (that seems so much more appropriate than "model") to be defaulted in each case.  (I prefer output parameters to be first to allow for cases just like this. ;-)

Obviously, similar arguments apply to unique_directory().

Arguably, the overloads that default the pattern argument would be more efficient if they were replaced by even more overloads without default arguments.  The reason is that the default path arguments force the compiler to create temporary path instances from the string literals every time the functions are called using the defaults.  With overloads and no defaults, the default pattern would be constructed in one place.

Thus:

   path
   unique_file(); // uses "%%%%-%%%%-%%%%-%%%%"

   path
   unique_file(path const & _pattern);

   // uses "%%%%-%%%%-%%%%-%%%%"
   path
   unique_file(system::error_code &);

   path
   unique_file(system::error_code &, path const & _pattern);

   path
   unique_file(path const & _parent, path const & _pattern);

   path
   unique_file(system::error_code &, path const & _parent,
      path const & _pattern);

> struct temporary_file
> {
>   // Uses unique_file. If delete_on_destruction is false you
>   // better go with the stand alone function

Indeed.  There's no reason to permit that here.  The purpose of this class is to create and destroy a temporary file.

>   temporary_file(const path& model="%%%%-%%%%-%%%%-%%%%",
>       const path& parent_path = temp_directory_path(),
>       bool delete_on_destruction = true) throws(...);

The argument order should be parent then pattern.  Furthermore, there should be constructor overloads to match the unique_file() overloads.

>   const path& path() const;
> };

While this combines creating and destroying a temporary file, I'm not convinced it is better than calling unique_file() and creating a scoped_file_remover.  The reason is that a temporary_file isn't a file object, so the name is misleading.  (The same argument could be applied to unique_file() not returning a file, but it isn't a class, so there are no instances, so we can be looser in that case.)  Another reason is that temporary_file provides a second means to the same end of creating a temporary file.

> struct temporary_directory
> {
>   // Uses unique_directory. If delete_on_destruction is false
>   // you better go with the stand alone function

No reason for the option.

>   temporary_directory(const path& model="%%%%-%%%%-%%%%-%%%%",
>      const path& parent_path = temp_directory_path(),
>      bool delete_on_destruction = true) throws(...);

Match the unique_directory() overloads.

>   const path& path() const;
> };

I think unique_directory()/scoped_directory_remover is likely a better scheme for the same reasons as for temporary_file.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.