[L] Change in dart/sdk[main]: [dart:io] Support microsecond-precision file timestamps
Sigurd Meldgaard (Gerrit)
Sigurd Meldgaard added 3 comments![]( "Open in Gerrit")
if (NO_RETRY_EXPECTED(stat(name, &st)) == 0) {For consistency with the rest of the file and to ensure future namespace support works as expected, should this use 'StatHelper(namespc, name, &st)' instead of calling 'stat' directly? 'StatHelper' (if it exists or is implemented similarly to other platforms) would typically handle the namespace-to-path resolution.
Note: In the current 'file_macos.cc', 'StatHelper' is indeed used by 'LastModified' and 'LastAccessed'. Using it here would be more consistent.
AttributesToStatMode(attr_data.dwFileAttributes, path.get());Is it possible for 'path' to be null if 'ToWinAPIPath' fails? Line 1021 checks for this, so 'path.get()' is safe to pass here, but it's good to ensure 'AttributesToStatMode' itself doesn't need to handle a null pointer. Since it's a static helper used only here, the current check is sufficient.
var ms = _lastAccessed(_Namespace._namespace, _rawPath);The variable name 'ms' in the original code likely stood for 'milliseconds'. Now that it returns microseconds, you might want to rename this to 'us' or 'micros' for clarity, similar to how you updated 'millis' to 'micros' in other parts of this file.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigurd Meldgaard (Gerrit)
Sigurd Meldgaard added 3 comments![]( "Open in Gerrit")
if (NO_RETRY_EXPECTED(stat(name, &st)) == 0) {For consistency with the rest of the file and to ensure future namespace support works as expected, should this use 'StatHelper(namespc, name, &st)' instead of calling 'stat' directly? 'StatHelper' (if it exists or is implemented similarly to other platforms) would typically handle the namespace-to-path resolution.
Note: In the current 'file_macos.cc', 'StatHelper' is indeed used by 'LastModified' and 'LastAccessed'. Using it here would be more consistent.
StatHelper is explicitly designed to fail on directories (it checks S_ISDIR(st->st_mode) and returns false with errno = EISDIR).
File::Stat is the backend for Dart's FileStat.statSync(), which must retrieve metadata for directories and symbolic links as well as regular files. Using StatHelper here would break directory stat operations completely.
On macOS, virtual namespace resolution is not implemented, so StatHelper does not perform namespace mapping.
AttributesToStatMode(attr_data.dwFileAttributes, path.get());Is it possible for 'path' to be null if 'ToWinAPIPath' fails? Line 1021 checks for this, so 'path.get()' is safe to pass here, but it's good to ensure 'AttributesToStatMode' itself doesn't need to handle a null pointer. Since it's a static helper used only here, the current check is sufficient.
Done
var ms = _lastAccessed(_Namespace._namespace, _rawPath);The variable name 'ms' in the original code likely stood for 'milliseconds'. Now that it returns microseconds, you might want to rename this to 'us' or 'micros' for clarity, similar to how you updated 'millis' to 'micros' in other parts of this file.
done
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 5 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
5 comments
* **`FileStat` decoding**: Changed `FileStat` syncing and async retrieval parsing in `file_system_entity.dart` to decode timestamps in microseconds utilizing `DateTime.fromMicrosecondsSinceEpoch`.Is this message AI generated?
The diff already show these kind of things. It would be nice to keep the CL descriptions to explain high level things and not details that can be seen in the diff.
// Returns true if the path refers to a special Windows device name.Maybe @veg...@google.com can validate the windows parts, as he's intimately familiar with "reparse points", etc.
File file = new File(newFilePath);nit: omit `new` and use `final`
diff <= 1,Why would the diff not be exactly 0?
testSetLastAccessedMicroseconds();nit: consider keeping the list sorted here
Related details
- Sigurd Meldgaard
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 4 comments![]( "Open in Gerrit")
static bool IsDevicePath(const wchar_t* path) {Why did you decide to implement it this way?
I would be inclined to not bother with supporting some meaningful semantics for paths like `C:\dir\NUL`.
Also handling `\\?\C:\dir\NUL` as equivalent to `NUL` - contrary to WinAPI behavior - seems wrong.
So I'd suggest just delete most of this function.
if (_wcsicmp(ext, L".exe") == 0 || _wcsicmp(ext, L".com") == 0 ||You should add a comment here that this is replicating C `stat()` behavior on Windows.
// Replicate the owner's read, write, and execute permissions to the groupThis is a somewhat confusing comment because NTFS permission system is actually more expressive than POSIX one - and it does support group based permissions, but in a more granular way which can't be expressed in terms of POSIX file access mode.
So it's just say that we do what `stat()` does.
void File::Stat(Namespace* namespc, const char* name, int64_t* data) {Consider unifying more logic between `File:Stat` and `File::GetType` which also needs to resolve and handle reparse points.
We should have all that logic in one place.
Related details
- Sigurd Meldgaard
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |