There is a long outstanding feature request, #1090 from Marcus Lindblom, requesting the following be added to Boost.Filesystem: class scoped_file { public: explicit scoped_file(const boost::filesystem::path& file) : m_path(file) {} ~scoped_file() { boost::filesystem::remove(m_path); } private: boost::filesystem::path m_path; }; My weakly held opinion is that this request should be rejected. Rationale: Easy enough for users to do themselves, so it doesn't justify the additional surface area it would add to of the library. Does anyone have a killer argument for adding it? --Beman
Beman Dawes wrote:
There is a long outstanding feature request, #1090 from Marcus Lindblom, requesting the following be added to Boost.Filesystem:
class scoped_file { public: explicit scoped_file(const boost::filesystem::path& file) : m_path(file) {}
~scoped_file() { boost::filesystem::remove(m_path); }
private: boost::filesystem::path m_path; };
My weakly held opinion is that this request should be rejected. Rationale: Easy enough for users to do themselves, so it doesn't justify the additional surface area it would add to of the library.
Does anyone have a killer argument for adding it?
Hi If the constructor would create the file I find this useful, even more if there is an option to create a unique file (using tmpfile, for instance). Temporary unique files are common and it would be nice to have a portable implementation. The name could be temporary_file instead of scoped_file. Please note that the current unique_path function is not very useful because the path is unique when it is generated but not necessarily when it is used. More generally, the library could include file creation, with an auto delete option and/or unique names. Just a comment, not a request :-) In its current version I think this class is prone to errors. Since the destructor removes the file, it might be confusing if the constructor creates it or not. The confusion increases if file creation is added to the library. Best regards Jorge
Jorge Lodos Vigil wrote:
Beman Dawes wrote:
There is a long outstanding feature request, #1090 from Marcus Lindblom, requesting the following be added to Boost.Filesystem:
class scoped_file { public: explicit scoped_file(const boost::filesystem::path& file) : m_path(file) {}
~scoped_file() { boost::filesystem::remove(m_path); }
private: boost::filesystem::path m_path; };
My weakly held opinion is that this request should be rejected. Rationale: Easy enough for users to do themselves, so it doesn't justify the additional surface area it would add to of the library.
It satisfies a not uncommon need. Boost.ScopeExit can be used to effect the same behavior: std::string const pathname(/* get the pathname */); BOOST_SCOPE_EXIT((&pathname)) { boost::filesystem::remove(pathname); } BOOST_SCOPE_EXIT_END That's not nearly as satisfying as: std::string const pathname(/* get the pathname */); boost::filesystem::scoped_file _(pathname); Alexandrescu's ScopeGuard provides another means to produce the same behavior.
Does anyone have a killer argument for adding it?
The obvious use is that one is creating a temporary file in a scope and wishes to ensure that it is deleted upon leaving that scope. I've done this sort of thing many times.
If the constructor would create the file I find this useful, even more if there is an option to create a unique file (using tmpfile, for instance). Temporary unique files are common and it would be nice to have a portable implementation. The name could be temporary_file instead of scoped_file. Please note that the current unique_path function is not very useful because the path is unique when it is generated but not necessarily when it is used.
I created just such a temporary file class for use in our unit test library. It uses OS-specific APIs to create a temporary file which it destroys on destruction. Obviously, such a class must provide normal file operations to be useful, whereas scoped_file is merely concerned with removing the file upon scope exit. Note that the interface of scoped_file is not as complete as it could be. Adding a release() member function would mark the file to not be deleted in the destructor. That would expand the usefulness of scoped_file to cases in which the file may be wanted beyond the current scope, leaving scoped_file to delete it when things fail. Another useful addition is a remove() member function to delete the file explicitly. That allows for exception propagation whereas the destructor cannot do so. (Whether anyone would ever bother calling remove() is another matter.)
In its current version I think this class is prone to errors. Since the destructor removes the file, it might be confusing if the constructor creates it or not. The confusion increases if file creation is added to the library.
Many things in C++ are prone to errors. They can still be useful. For the intended purpose, I think the real problem with scoped_file is the name. scoped_file_remover would be clear and avoid any confusion as to its use. _____ Rob Stewart robert.stewart@sig.com 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.
Robert Stewart wrote:
Jorge Lodos Vigil wrote:
Beman Dawes wrote:
There is a long outstanding feature request, #1090 from Marcus Lindblom, requesting the following be added to Boost.Filesystem:
class scoped_file { public: explicit scoped_file(const boost::filesystem::path& file) : m_path(file) {}
~scoped_file() { boost::filesystem::remove(m_path); }
private: boost::filesystem::path m_path; };
My weakly held opinion is that this request should be rejected. Rationale: Easy enough for users to do themselves, so it doesn't justify the additional surface area it would add to of the library.
If the constructor would create the file I find this useful, even more if there is an option to create a unique file (using tmpfile, for instance). Temporary unique files are common and it would be nice to have a portable implementation. The name could be temporary_file instead of scoped_file. Please note that the current unique_path function is not very useful because the path is unique when it is generated but not necessarily when it is used.
I created just such a temporary file class for use in our unit test library. It uses OS-specific APIs to create a temporary file which it destroys on destruction. Obviously, such a class must provide normal file operations to be useful, whereas scoped_file is merely concerned with removing the file upon scope exit.
I don't believe a temporary file class _must_ provide any file operation, it would be very useful by just ensuring that the created temporary file is unique and exposing its name. This is the problem that currently cannot be solved with boost::filesystem. If temporary files are not created together with the temporary path there is always a risk that the path is used between the time it was found and the file was created. To ensure _file_ uniqueness you must use functions like tmpfile. Best regards Jorge
Jorge Lodos Vigil wrote:
Robert Stewart wrote:
I created just such a temporary file class for use in our unit test library. It uses OS-specific APIs to create a temporary file which it destroys on destruction. Obviously, such a class must provide normal file operations to be useful, whereas scoped_file is merely concerned with removing the file upon scope exit.
I don't believe a temporary file class _must_ provide any file operation, it would be very useful by just ensuring that the created temporary file is unique and exposing its name.
If temporary_file creates the file but exposes no file operations, then one must call a pathname() function, close the temporary file in a way that doesn't delete it, open the pathname using another class (std::fstream, boost::filesystem::fstream, etc.), and then ensure that the other object is destroyed before the temporary_file instance removes the file (at least on Windows). That's hardly a pleasing interface. A temporary_file class that includes file operations is, of course, beyond the current scope of Filesystem unless temporary_file were another variation of the file streams classes.
This is the problem that currently cannot be solved with boost::filesystem. If temporary files are not created together with the temporary path there is always a risk that the path is used between the time it was found and the file was created. To ensure _file_ uniqueness you must use functions like tmpfile.
It is enough to have a function that creates a zero-length, temporary file and returns its pathname. That probably fits better with Filesystem's design anyway. Such a function would have to implement a loop that calls unique_path(), tries to create a file with that pathname, and repeats some number of times until the creation succeeds. Then, it would close the file and return the pathname. That code is non-trivial and would be a helpful addition. This still leaves scoped_file_remover an open question. _____ Rob Stewart robert.stewart@sig.com 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.
On Wed, Oct 12, 2011 at 2:11 PM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
It is enough to have a function that creates a zero-length, temporary file and returns its pathname. That probably fits better with Filesystem's design anyway. Such a function would have to implement a loop that calls unique_path(), tries to create a file with that pathname, and repeats some number of times until the creation succeeds. Then, it would close the file and return the pathname. That code is non-trivial and would be a helpful addition.
That doesn't seem right either. It shouldn't be necessary to close and reopen the file. Olaf
Olaf van der Spek wrote:
On Wed, Oct 12, 2011 at 2:11 PM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
It is enough to have a function that creates a zero-length, temporary file and returns its pathname. That probably fits better with Filesystem's design anyway. Such a function would have to implement a loop that calls unique_path(), tries to create a file with that pathname, and repeats some number of times until the creation succeeds. Then, it would close the file and return the pathname. That code is non-trivial and would be a helpful addition.
That doesn't seem right either. It shouldn't be necessary to close and reopen the file.
How would you transfer the open file handle to std::fstream, for example? _____ Rob Stewart robert.stewart@sig.com 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.
On Wed, Oct 12, 2011 at 2:24 PM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
That doesn't seem right either. It shouldn't be necessary to close and reopen the file.
How would you transfer the open file handle to std::fstream, for example?
I think std::fstream doesn't support that. But that seems a limitation of std::fstream. Olaf
On Oct 12, 2011, at 11:59 AM, Olaf van der Spek wrote:
On Wed, Oct 12, 2011 at 2:24 PM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
That doesn't seem right either. It shouldn't be necessary to close and reopen the file.
How would you transfer the open file handle to std::fstream, for example?
I think std::fstream doesn't support that. But that seems a limitation of std::fstream.
which is addressed by boost::iostreams::stream<boost::iostreams::file_descriptor_sink>
On Wed, Oct 12, 2011 at 8:11 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Jorge Lodos Vigil wrote:
Robert Stewart wrote:
I created just such a temporary file class for use in our unit test library. It uses OS-specific APIs to create a temporary file which it destroys on destruction. Obviously, such a class must provide normal file operations to be useful, whereas scoped_file is merely concerned with removing the file upon scope exit.
I don't believe a temporary file class _must_ provide any file operation, it would be very useful by just ensuring that the created temporary file is unique and exposing its name.
If temporary_file creates the file but exposes no file operations, then one must call a pathname() function, close the temporary file in a way that doesn't delete it, open the pathname using another class (std::fstream, boost::filesystem::fstream, etc.), and then ensure that the other object is destroyed before the temporary_file instance removes the file (at least on Windows). That's hardly a pleasing interface.
A temporary_file class that includes file operations is, of course, beyond the current scope of Filesystem unless temporary_file were another variation of the file streams classes.
This is the problem that currently cannot be solved with boost::filesystem. If temporary files are not created together with the temporary path there is always a risk that the path is used between the time it was found and the file was created. To ensure _file_ uniqueness you must use functions like tmpfile.
It is enough to have a function that creates a zero-length, temporary file and returns its pathname. That probably fits better with Filesystem's design anyway. Such a function would have to implement a loop that calls unique_path(), tries to create a file with that pathname, and repeats some number of times until the creation succeeds. Then, it would close the file and return the pathname. That code is non-trivial and would be a helpful addition.
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. --Beman
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); struct temporary_file { // Uses unique_file. If delete_on_destruction is false you better go with the stand alone function temporary_file(const path& model="%%%%-%%%%-%%%%-%%%%", const path& parent_path = temp_directory_path(), bool delete_on_destruction = true) throws(...); const path& path() const; }; struct temporary_directory { // Uses unique_directory. If delete_on_destruction is false you better go with the stand alone function temporary_directory(const path& model="%%%%-%%%%-%%%%-%%%%", const path& parent_path = temp_directory_path(), bool delete_on_destruction = true) throws(...); const path& path() const; }; Thank you. Best regards Jorge
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@sig.com 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.
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
participants (5)
-
Beman Dawes -
Jorge Lodos Vigil -
Kim Barrett -
Olaf van der Spek -
Stewart, Robert