[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Volodymyr,

Thanks for working on this, really nice work with the tests! Comments below.

> - No support for 'fallthrough' in crash reproducer.

That'd be nice to have at some point to make sure we never escape into the real fs.

> - Current way of working with modules in VFS "root" is clunky and error-prone.

Why?

> Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Maybe add FIXMEs for it?



================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:934
   RedirectingFileSystem &FS;
   RedirectingDirectoryEntry::iterator Current, End;
+  bool IsExternalFSCurrent;
----------------
Can you add comments to these new members? Specifically, what's the purpose of `IsExternalFSCurrent` and how it connects  to `ExternalFS`


================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:940
 
-  std::error_code incrementImpl();
+  std::error_code incrementExternal();
+  std::error_code incrementContent(bool IsFirstTime);
----------------
Same for these methods


================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:1738
+ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path,
+                                              bool ShouldCheckExternalFS) {
   ErrorOr<Entry *> Result = lookupPath(Path);
----------------
Is passing `ShouldCheckExternalFS ` really necessary? Why checking the status of the member isn't enough?


================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:2041
+      IsExternalFSCurrent(false), ExternalFS(ExternalFS) {
+  EC = incrementImpl(true);
 }
----------------
`incrementImpl(true/*IsFirstTime*/)`


================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:2045
 std::error_code VFSFromYamlDirIterImpl::increment() {
-  assert(Current != End && "cannot iterate past end");
-  ++Current;
-  return incrementImpl();
+  return incrementImpl(false);
+}
----------------
`incrementImpl(false/*IsFirstTime*/)`


https://reviews.llvm.org/D50539