Use filesystem::path instead of manual string tinkering #1072
Conversation
|
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. |
|
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. |
|
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, 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. |
|
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! |
|
ACK |
|
This needs to get in, before #1066, more ACKs please ^^. |
|
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... |
|
|
||
| // This is only necessary as long as we want boost::filesystem v2 compatibility | ||
| #if defined(BOOST_FILESYSTEM_VERSION) && BOOST_FILESYSTEM_VERSION >= 3 | ||
| # define dir_cstr() string().c_str() |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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*.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
Use filesystem::path instead of manual string tinkering
Use filesystem::path instead of manual string tinkering
update protocol-documentation.md
fix spurious disconnect due to block download crowding out getheaders
9695100 Update welcomecontentwidget.ui (Jeffrey) Pull request description: Changes made, Mostly due to SwiftX being disabled and no privacy option yet for this release. ACKs for top commit: Fuzzbawls: utACK 9695100 Tree-SHA512: 4d91fd09f18a5a815019b009a3fb4cf36685ad99a75f1a866046c8cfd7be50c0606069eee5710c0fc9e4963d28d4b4677d78060e8f15a08e74ebcb3b02209a9d
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.