Use filesystem::path instead of manual string tinkering #1072

pull sipa wants to merge 1 commits into bitcoin:master from sipa:boostpaths changing 6 files +95 −131

sipa commented at 10:02 PM on April 9, 2012: member

Where possible, use boost::filesystem::path instead of std::string or char* for filenames. This avoids a lot of manual string tinkering, in favor of path::operator/.

GetDataDir is also reworked significantly, it now only keeps two cached directory names (the network-specific data dir, and the root data dir), which are decided through a parameter instead of pre-initialized global variables.

Finally, remove the "upgrade from 0.1.5" case where a debug.log in the current directory has to be removed.
jgarzik commented at 10:41 PM on April 9, 2012: contributor

I will leave ACK/NAK to others, but I will note... this Boost feature is occasionally used as an example of C++ abuse. It is "cute" to build pathnames using the "/" operator, but such usage is decidedly non-standard for the "/" operator and therefore confusing to the uninitiated reading the code.
sipa commented at 10:53 PM on April 9, 2012: member

I wouldn't mind using a longer function name, or operator to accomplish the same. The point is that this operator constrcuts paths in a platform-independent way.

The closest alternative is extracting the path as a string, use platform-dependent code to select the correct separator, add the extra path component, and check for various issues such as double slashes, "." and "..", appending a final slash or not, ... All this functionality is already implemented by boost::filesystem::path. You may not agree to its naming conventions, but I certainly see no reason to reimplement it ourself.
laanwj commented at 6:43 AM on April 10, 2012: member

I agree with @jgarzik on that this is "cute c++ abuse" by boost. Then again, the standard library << and >> for stream operators?!? is not much better. And just like those, / has no standard use for strings.

This is an upstream issue way outside the scope of bitcoin. We cannot decide the interfaces of the upstream libraries we use. It's a good idea to just use the functionality IMO as it provides a platform-independent way of building paths.
Diapolo commented at 7:17 AM on April 10, 2012: none

I like your changes and prefer to get them merged to master before my #1066 gets in, as I will likely have to rebase and rework some parts, but the other way around it would be nonsense :)!

So definately ACK!
laanwj commented at 11:02 AM on April 10, 2012: member

ACK
Diapolo commented at 5:47 PM on April 10, 2012: none

This needs to get in, before #1066, more ACKs please ^^.
sipa commented at 6:00 PM on April 10, 2012: member

The helper functions to convert boost::filesystem::path to const char* risked returning something pointing to a destroyed temporary on windows. I've replaced them now with slightly uglier macros that should be safe...

Use filesystem::path instead of manual string tinkering

Where possible, use boost::filesystem::path instead of std::string or
char* for filenames. This avoids a lot of manual string tinkering, in
favor of path::operator/.

GetDataDir is also reworked significantly, it now only keeps two cached
directory names (the network-specific data dir, and the root data dir),
which are decided through a parameter instead of pre-initialized global
variables.

Finally, remove the "upgrade from 0.1.5" case where a debug.log in the
current directory has to be removed.

ee12c3d60c

in src/util.h:None in 1c05da5e30 outdated
```
 303 | @@ -304,9 +304,18 @@ class CMutexLock
 304 |  // (secure_allocator<> is defined in serialize.h)
 305 |  typedef std::basic_string<char, std::char_traits<char>, secure_allocator<char> > SecureString;
 306 |  
 307 | -
 308 | -
 309 | -
 310 | +// This is only necessary as long as we want boost::filesystem v2 compatibility
 311 | +#if defined(BOOST_FILESYSTEM_VERSION) && BOOST_FILESYSTEM_VERSION >= 3
 312 | +#    define dir_cstr() string().c_str()
```
gavinandresen commented at 12:22 AM on April 11, 2012:

I really don't like #defines like this in an included-by-everything file like util.h.

It is reasonably likely at some point somebody will write or use a class with a dir_str() method and will spend a long time trying to figure out they're getting weird compiler errors.

Can we just use .string()/.string.c_str(), and live with "ugly" paths if you're running on Windows and not using BOOST_FILESYSTEM_VERSION >= 3 ?

PS: All of this is in my "not important enough to care" category...

sipa commented at 12:31 AM on April 11, 2012:

Does that work? It seemed to me that filesystem v2's path::string() could return a string of wchar_t, so its .c_str() would not create a const char*.

sipa commented at 12:40 AM on April 11, 2012:

By the way: yes, it's ugly - I'd prefer to either get rid of v2 altogether (but that would break support with boost <1.44, which is apparently still used in some places), or get rid of the conversions to C strings entirely. But that's probably not for anytime soon, as even boost's own lockfile implementation requires a const char* argument.

sipa commented at 1:24 AM on April 11, 2012:

As the windows gitian builds use 1.47, I guess we can safely ignore the case of pre-v3 boost fs on windows. Updated to use .string().c_str() everywhere.

gavinandresen commented at 1:36 AM on April 11, 2012:

Yay! ACK
sipa referenced this in commit ca2c1cb446 on Apr 11, 2012
sipa merged this on Apr 11, 2012
sipa closed this on Apr 11, 2012
coblee referenced this in commit 46973487fe on Jul 17, 2012
sipa deleted the branch on Jun 23, 2017
suprnurd referenced this in commit ef87e4ccb1 on Dec 5, 2017
sanch0panza referenced this in commit 6f68f680e3 on May 17, 2018
lateminer referenced this in commit 1e0cea53e8 on Nov 3, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#25202 log: Use severity-based logging for leveldb/libevent messages, reverse LogPrintLevel order