This is an archive of the discontinued LLVM Phabricator instance.

[VFS] InMemoryFilesystem's UniqueIDs are a function of path and content.
ClosedPublic

Authored by sammccall on Sep 29 2021, 6:26 AM.

Details

Summary

This ensures that re-creating "the same" FS results in the same UIDs for files.
In turn, this means that creating a clang module (preamble) using one in-memory
filesystem and consuming it using another doesn't create duplicate FileEntrys
for files that are the same in both FSes.

It's tempting to give the creator control over the UIDs instead. However that
requires fiddly API changes, e.g. what should the UIDs of intermediate
directories be?
This change is more "magic" but seems safe given:

  • InMemoryFilesystem is used in testing more than production
  • comparing UIDs across filesystems is unusual
  • files with the same path and content are usually logically equivalent

(The usual reason for re-creating virtual filesystems rather than reusing them
is that typical use involves mutating their CWD and so is not threadsafe).

Diff Detail

Event Timeline

sammccall created this revision.Sep 29 2021, 6:26 AM
sammccall requested review of this revision.Sep 29 2021, 6:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 6:26 AM

thanks, i think it LG, with a couple questions :)

llvm/include/llvm/Support/VirtualFileSystem.h
433 ↗(On Diff #375859)

can't we just have these as implementation details? what's the point of them being members?

llvm/lib/Support/VirtualFileSystem.cpp
724–726

nit: maybe use an empty filename instead? (as we feed into the stat)

748

why the need for the extra 1 in the end?

sammccall updated this revision to Diff 375880.Sep 29 2021, 7:20 AM
sammccall marked 3 inline comments as done.

Simplify

kadircet accepted this revision.Sep 29 2021, 7:39 AM

thanks, lgtm!

This revision is now accepted and ready to land.Sep 29 2021, 7:39 AM

Awesome, thanks so much!

This revision was landed with ongoing or failed builds.Sep 29 2021, 2:24 PM
This revision was automatically updated to reflect the committed changes.