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

[Zachary Turner via lldb-commits]

zturner 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);
----------------
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.


================
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)
----------------
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?


================
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),
----------------
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.


https://reviews.llvm.org/D25392