This is an archive of the discontinued LLVM Phabricator instance.

Remove TimeValue usage from FileSpec.h
ClosedPublic

Authored by labath on Oct 7 2016, 9:23 PM.

Details

Summary

The only usage there was in GetModificationTime(). I also took the opportunity
to move this function from FileSpec to the FileSystem class - since we are
using FileSpecs to also represent remote files for which we cannot (easily)
retrieve modification time, it makes sense to make the decision to get the
modification time more explicit.

The new function returns a std::duration::time_point. To aid the transition
from TimeValue, I have added a constructor to it which enables implicit
conversion from a time_point.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 74014.Oct 7 2016, 9:23 PM
labath retitled this revision from to Remove TimeValue usage from FileSpec.h.
labath updated this object.
labath added reviewers: clayborg, zturner.
labath added a subscriber: lldb-commits.
labath added inline comments.
source/Host/common/FileSystem.cpp
103 ↗(On Diff #74014)

The conversion from struct timespec will probably be necessary in some other places as well.

zturner added inline comments.Oct 8 2016, 8:42 AM
include/lldb/Host/FileSystem.h
69–71 ↗(On Diff #74014)

I wonder if it would be worth defining some typedefs in LLVM's Chrono.h that Mehdi is adding to make things like this less verbose. For example:

llvm::nanosecond_time_point
llvm::microsecond_time_point
llvm::nanosecond_duration
llvm::microsecond_duration

Now, with all that aside, I'm not sure we actually need this function here (or many of the other functions for that matter). LLVM has llvm::support::fs::status() which will return you a file_status object which contains the modification time. I wonder if we should use that instead. It currently uses an llvm::TimeValue, but the conversion to chrono could happen at that level presumably.

include/lldb/Host/TimeValue.h
37–38 ↗(On Diff #74014)

Is there any reason to explicitly prefer a nanosecond clock here as opposed to std::chrono::system_clock::duration? For any system which doesn't have nanosecond resolution, using a nanosecond clock is pointless, and if some theoretical system had finer resolution, then using a nanosecond clock would be limiting. So how about just std::chrono::time_point<std::chrono::system_clock> and let the final argument be deduced?

source/Core/Module.cpp
238–245 ↗(On Diff #74014)

Whenever I touch code like this, I've been goign through and deleting all the explicit constructor initializers and putting them in the header. All the ones of class type can just disappear, and the bools can be initialized inline at the point of declaration.

labath added inline comments.Oct 8 2016, 9:18 AM
include/lldb/Host/FileSystem.h
69–71 ↗(On Diff #74014)

That's a good point. I can check how easy it would be to do that. I'd like to avoid refactoring the filesystem code just to get the time value conversion done though.

include/lldb/Host/TimeValue.h
37–38 ↗(On Diff #74014)

If you let this argument be less precise you will get conversion errors if someone tries to pass in a time point which has higher precision (seconds are implicitly convertible to nanoseconds, but not the other way). And struct timespec already (theoretically) has nanosecond precision, so we would have to make an explicit choice to lose the precision at some level.

chrono::system_clock::time_point has the precision at which the host system clock hands out timestamps, but that doesn't mean it's impossible to get higher precision timestamps, particularly if we start dealing with remote systems.

source/Core/Module.cpp
238–245 ↗(On Diff #74014)

sounds like a good idea.

labath added inline comments.Oct 8 2016, 4:23 PM
include/lldb/Host/TimeValue.h
37–38 ↗(On Diff #74014)

I've hit one more annoying issue system_clock::to_time_t will not accept a time point with precision greater than native system clock precision. Sort of makes sense, although it's pretty pointless since time_t has only second precision on all reasonable platforms. This means, if we use nanosecond precision everywhere, we would have to cast, or roll our own version of to_time_t. If we use native system clock precision, we lose the ability to represent nanoseconds in some cases (and there are filesystems storing timestamps with nanosecond precision), cannot represent struct timespec nor the current TimeValues correctly, and our time functions would behave differently depending on the host os. I'd go with the first option as it seems to have less drawbacks.

I'll try to come up with a coherent proposal on the llvm side. Since it seems we'll be putting these things there, I feel I should start by porting the usage in llvm anyway.

clayborg accepted this revision.Oct 10 2016, 12:50 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Oct 10 2016, 12:50 PM
labath updated this revision to Diff 76573.Nov 1 2016, 9:09 AM
labath marked 2 inline comments as done.
labath edited edge metadata.
  • cleanup constructors
  • use llvm's stat() implementation
include/lldb/Host/FileSystem.h
69–71 ↗(On Diff #74014)

I've changed this to use llvm::sys::fs::status() underneath (which now uses chrono). I am still keeping this function, as it has a somewhat different interface (takes FileSpec, returns an empty time point in case of an error), and I don't want to update all callers now.

This revision was automatically updated to reflect the committed changes.