Boost logo

Boost :

Subject: Re: [boost] [filesystem] scoped_file feature request?
From: Jorge Lodos Vigil (lodos_at_[hidden])
Date: 2011-10-13 08:57:25


Robert Stewart wrote:

> Jorge Lodos Vigil wrote:
> >
> > 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 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.
>

The second overload is a copy/paste of unique_path, as you noticed. I thought it was important to follow the unique_path precedent. I'm sure there were reasons to select this prototype for unique_path, but other than that I have no arguments. What you say is reasonable.

> 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().
>

Sure, rapid fingers issue :-). I agree. The third and fourth overloads are an attempt to allow creation of a temporary file in a specified folder. As the pattern can be an invalid filename, I'm not sure if filesystem specification allows operations with invalid filenames. If this is part of the specification then these overloads may be dropped. Must filesystem::operator/= work even if the parameter is an invalid path?

> 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);
>

I agree, but I think unique_path compatibility is an issue. On the other hand, unique_path may be also overloaded. I believe more opinions are needed here, specially Beman's.

> > 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.
>

Agreed, what is important here is that "there should be constructor overloads to match the unique_file() overloads". The class must also be prevented to be copy constructible. An attach method could be added as well to satisfy the initial scoped_file request :-).

> > 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.
>

Trying to find an appropriate name for this class was/is difficult for me, so I guess you are right. The best alternative I could find was temporary_file_path, but it is also confusing, temporary_file seemed better to me.
>
> I think unique_directory()/scoped_directory_remover is likely a better scheme for the same reasons as for temporary_file.

I don't have a strong opinion on this, just that this is a so commonly used pattern that having it in a library would be very useful.

Best regards
Jorge


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk