[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

Jonas Devlieghere via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Nov 2 13:24:04 PDT 2018

JDevlieghere added a comment.

In https://reviews.llvm.org/D54020#1285799, @labath wrote:

> Ok, I think we're on the same page here. What I was wondering is that given this lack of generality (this operation would only be supported on `DiskBackedFilesystem`, and not all other kinds of file systems), whether it is not better to just do a custom solution instead of going through the VFS layer (thereby removing one layer, since now we have `lldb_private::FileSystem` sitting on top of `llvm::VirtualFileSystem`, sitting on top of the real thing). E.g., if we know we can restrict ourselves to the case where the on disk layout matches the real system, then all filesystem operations can be implemented very trivially via something like:
>   auto virtual_op(const Twine &Path, ...) { return real_op(Prefix+Path, ...); }
> I am not sure whether this is a good idea (there's definitely a case to be made for reusing existing infrastructure), but it is something to think about.

Thanks Pavel, I really appreciate you thinking this through with me! I understand what you're saying but I still think the VFS is worthwhile:

- The lack of generality I brought up only applies to in-memory file systems. Every VFS implementation that's backed by disk would still work.
- The only reason hard-coding the vfs implementation (because that's essentially what the prefix thing is) is the coincidence that no prefix equals the real file system.
- We could have a prefix VFS implementation that's really simple and self-contained without cluttering the LLDB `FileSystem` class with details.

In the end I believe the indirection is more of a feature than a bug. I'm worried that, if we do our own thing, we'll end up in this situation where we have something in LLDB that looks really similar to something in LLVM but isn't quite the same. As soon as we want to do more complex than just prepending a prefix we'll need an abstraction anyway, so it might as well be the thing from LLVM.

Comment at: source/Host/common/FileSystem.cpp:253
+static int GetOpenFlags(uint32_t options) {
+  const bool read = options & File::OpenOptions::eOpenOptionRead;
+  const bool write = options & File::OpenOptions::eOpenOptionWrite;
labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > `File::eOpenOptionRead` should be sufficient, no?
> > Sorry, I don't know what you mean?
> I meant to remove the superfluous `OpenOptions` qualification, As this is a non-class enum, it is not necessary, and it doesn't help clarity, since the enum value already contains the type name. None of the existing code uses this kind of qualification.
Got it, thanks!


More information about the lldb-commits mailing list