[Lldb-commits] [PATCH] D25392: Remove TimeValue usage from FileSpec.h

[Pavel Labath via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Host/FileSystem.h:69-71
+  static std::chrono::time_point<std::chrono::system_clock,
+                                 std::chrono::nanoseconds>
+  GetModificationTime(const FileSpec &file_spec);
----------------
zturner wrote:
> 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.
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.


================
Comment at: include/lldb/Host/TimeValue.h:37-38
   explicit TimeValue(uint32_t seconds, uint64_t nanos = 0);
+  TimeValue(std::chrono::time_point<std::chrono::system_clock,
+                                    std::chrono::nanoseconds>
+                point)
----------------
zturner wrote:
> 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?
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.


================
Comment at: source/Core/Module.cpp:238-245
+    : m_mutex(), m_mod_time(FileSystem::GetModificationTime(file_spec)),
+      m_arch(arch), m_uuid(), m_file(file_spec), m_platform_file(),
+      m_remote_install_file(), m_symfile_spec(), m_object_name(),
+      m_object_offset(object_offset), m_object_mod_time(), m_objfile_sp(),
+      m_symfile_ap(), m_type_system_map(), m_source_mappings(), m_sections_ap(),
+      m_did_load_objfile(false), m_did_load_symbol_vendor(false),
+      m_did_parse_uuid(false), m_file_has_changed(false),
----------------
zturner wrote:
> 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.
sounds like a good idea.


https://reviews.llvm.org/D25392