[PATCH] D144172: [Support] [Windows] Don't check file access time in equivalent(file_status, file_status)

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D144172#4135280 <https://reviews.llvm.org/D144172#4135280>, @hans wrote:

> In D144172#4134306 <https://reviews.llvm.org/D144172#4134306>, @mstorsjo wrote:
>
>> We would need to extend the `file_status` struct, https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/include/llvm/Support/FileSystem.h#L225-L238, to include this. That's no problem. But we'd also need to extend `fs::UniqueID`, https://github.com/llvm/llvm-project/blob/llvmorg-17-init/llvm/include/llvm/Support/FileSystem/UniqueID.h#L26-L49, with that, and that seems to be much more annoying. We probably can't rely on 128 bit integers being available everywhere LLVM itself runs (since it can run on e.g. 32 bit hosts).
>
> That's annoying.
>
> I suppose we could add another uint64_t for the high bits in UniqueID, but then we'd have to propagate it to everyone consuming UniqueIDs. I don't know how annoying that would be.

I tried to grep around in the monorepo, and it seems like it would be significantly annoying to do that - lots of things rely on `UniqueID`s consisting of a pair only, and the data types being `uint64_t`.

We could of course just keep comparing individual fields in the `sys::fs::equivalent()` function, like we do now, instead of factorizing it to use `getUniqueID()` - but that's not really a complete fix, since `getUniqueID()` is used by lots of code too.

> Maybe it would be enough to hash combine the high bits with the low :-]

Hah, I guess that could be one way to go about it ;-)

One would hope that the implementations of file systems that use 128 bit IDs is such that it primarily uses the bits that map to `FileIndexLow`/`FileIndexHigh`, so that unless you actually do have more than 2^64 files, you won't run into issues by just using the older fields.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144172/new/

https://reviews.llvm.org/D144172