[PATCH] Add a VFSFromYAML class and a parser to create it

[Dmitri Gribenko]

  > Can this wait until we have more practical experience with the file format? Things in docs/ tend to have better documentation that what I've written here, and I don't want to put a lot of effort into this until we're sure this is the right format.

  OK, but I was asking about moving what is already in comments to .rst.  It is easier to maintain .rst IMHO, but it is up to you.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:302-303
@@ +301,4 @@
+      : ExternalFS(ExternalFS) {
+    llvm::Triple Triple(LLVM_HOST_TRIPLE);
+    CaseSensitive = Triple.isOSDarwin() || Triple.isOSWindows();
+  }
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > I don't think it is a good idea to make this decision here, in a low-level library, especially by calling into LLVM.  Having a single default of 'case-sensitive' is better, IMHO -- at least, VFS mapping files will be interpreted the same way on all platforms.
> But then the VFS file is interpreted differently from the host file system by default.  That seems just as bad.  For example, say I map over /Foo, on Darwin, and the user specifies a path in /foo.  Then we will get the real file system.
One can configure the FS to be case-sensitive on Darwin.

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:229
@@ +228,3 @@
+
+TEST(VirtualFileSystemTest, basicVFSFromYAML) {
+  IntrusiveRefCntPtr<vfs::FileSystem> FS;
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > It would be nice to also check that we produce at least a single diagnostic in this case.
> > 
> > Actually it would be really good if we had one test for every diagnostic we produce.
> I was planning to do this once I have added the clang option, and can use lit+FileCheck.  Doing diagnostic checks in the unit tests seems painful.
Could we just check the number of diagnostics?

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:296
@@ +295,3 @@
+
+TEST(VirtualFileSystemTest, caseSensitive) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > Test name should probably be 'CaseInsensitive'.
> Yes, will do.  I assume you mean 'caseInsenstive' (lower case c).
No, I meant with an uppercase C.  Please take a look at the list of all tests -- in most cases, the test case name starts from an uppercase letter as well: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-debian-fast/builds/12604/steps/test/logs/stdio

================
Comment at: lib/Basic/VirtualFileSystem.cpp:254
@@ +253,3 @@
+///   <optional configuration>
+///   'roots': [
+///              <directory entries>
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > If we want 'roots' to model Windows drives and similar things, then we also need some kind of label for each root.
> Why? I'm not intimately familiar with Windows file systems.  I thought that the 'name' field would be sufficient.
Oh, I see -- your design was that name of the top-level directory is the drive name.  I guess that could work as well (although it is non-uniform with *nix files having '/' as the first name).

================
Comment at: lib/Basic/VirtualFileSystem.cpp:705
@@ +704,3 @@
+  // dev_t value from the OS.
+  return UniqueID(std::numeric_limits<uint64_t>::max(), ID);
+}
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > This will only work correctly only as long as only a single YAML-based VFS exists, right?..
> It will 'work' for any number of VFSes, since the unique IDs will never collide (globally increasing file id prevents this).  The files/directories will all have the same 'device' in the unique ID, but that should be fine, since I don't think userspace programs are able to infer anything from that number anyway, except that together with the inode number they provide a unique file identifier.
Oh, I see, UID is static, and not per-VFS.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:674-675
@@ +673,4 @@
+    Status S = DE->getStatus();
+    S.setName(PathStr);
+    S.setExternalName(PathStr);
+    return S;
----------------
Ben Langmuir wrote:
> Dmitri Gribenko wrote:
> > Why do we overwrite external name with the virtual path?
> It's not being overwritten per se, since we initialized the name and external name fields to "".  It would be better to save these in the status object once, but when we create the directory entry, we don't have an easy way to get the entire path, only the last component of it.
> 
> As for why we provide a virtual path in ExternalName, the reason is that ExternalName is supposed to be a usable string in all cases, so no one has to ask, is there an external name? if not, use 'name'.  This means that ExternalName == Name, *except* when we are dealing with a file that is mapped to disk (using 'external-contents').
Understood.



http://llvm-reviews.chandlerc.com/D2835