Skip to content

ts_file: Simple sketch of std::filesystem for internal use.#4206

Merged
dragon512 merged 1 commit intoapache:masterfrom
SolidWallOfCode:ts_file
Sep 20, 2018
Merged

ts_file: Simple sketch of std::filesystem for internal use.#4206
dragon512 merged 1 commit intoapache:masterfrom
SolidWallOfCode:ts_file

Conversation

@SolidWallOfCode
Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode commented Sep 6, 2018

Having attempted and failed to get std::filesystem ready (it does not appear to be fully operational), this is a minimalist version that supplies only methods that are of immediate use. This is not intended to be a full replacement because that would be a huge amount of work and a waste because we will switch to std::filesystem on the next major version where it is available. For the same reason, the implementation style of this is modeled closely on std::filesystem to make the switch as easy as possible. An further implication of this is that no functionality not in std::filesystem should be (1) segregated and clearly marked and (2) built on top of std::filesystem compatible mechanisms. This is temporary expedient.

P.S. Note that the semantics of the path operator "/" changed significantly between std::experimental::filesystem and std::filesystem. This implements the latter.

This is based on work done for the Cache Tool, in "File.h" and "File.cc". Those were very rough and this is a recasting of those in the std::filesystem style. This is something that would be quite helpful in the YAML conversion work, and obviously in the Cache Tool as well.

@SolidWallOfCode SolidWallOfCode added this to the 9.0.0 milestone Sep 6, 2018
@SolidWallOfCode SolidWallOfCode self-assigned this Sep 6, 2018
@nqyy
Copy link
Copy Markdown
Contributor

nqyy commented Sep 10, 2018

Looks nice. It might be a good use for the Layout class. When can we get this in?

@SolidWallOfCode
Copy link
Copy Markdown
Member Author

Soon, hopefully. @bryancall made the point that this should not be too similar to std::filesystem because subtle, unnoticed differences can be worse that obvious large ones. Pondering that, I was original conflicted over having this in ts::file but that might actually be better for this reason because it will require manual intervention to change and any such issues can be dealt with at that time, and in an incremental fashion.

Comment thread src/tscore/ts_file.cc Outdated
return fs._stat.st_mode & S_IFMT;
}

off_t
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard uses std::uintmax_t
We should use the same value

Comment thread include/tscore/ts_file.h Outdated
using self_type = path;

public:
static constexpr char SEPARATOR = '/';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be or add

typedef char value_type;
typedef basic_string<value_type> string_type;
static constexpr value_type preferred_separator = '/';

SEPARATOR should be removed or we should avoid usage of it outside this class/function set. Maybe change it to _SEPARATOR or use preferred_separator.

Comment thread include/tscore/ts_file.h Outdated
inline bool
path::is_absolute() const
{
return !_path.empty() && '/' == _path[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use the preferred_separator or SEPERATOR or have a _is_seperator function/macro

@dragon512 dragon512 merged commit 8c6be99 into apache:master Sep 20, 2018
@gtenev gtenev mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants