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

[Dmitri Gribenko]

  We also need to document how our case-insensitive matching interacts with Unicode.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:674-675
@@ +673,4 @@
+    Status S = DE->getStatus();
+    S.setName(PathStr);
+    S.setExternalName(PathStr);
+    return S;
----------------
Why do we overwrite external name with the virtual path?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:705
@@ +704,3 @@
+  // dev_t value from the OS.
+  return UniqueID(std::numeric_limits<uint64_t>::max(), ID);
+}
----------------
This will only work correctly only as long as only a single YAML-based VFS exists, right?..

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:229
@@ +228,3 @@
+
+TEST(VirtualFileSystemTest, basicVFSFromYAML) {
+  IntrusiveRefCntPtr<vfs::FileSystem> FS;
----------------
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.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:378-381
@@ +377,6 @@
+
+    bool hasName = false;
+    bool hasKind = false;
+    bool hasContents = false; // external or otherwise
+    bool hasExternalContents = false;
+    std::vector<Entry *> EntryArrayContents;
----------------
Variable names should start with an uppercase letter.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:397-398
@@ +396,4 @@
+      if (Key == "name") {
+        if (!parseScalarString(I->getValue(), Value, Buffer))
+          return NULL;
+        hasName = true;
----------------
Missing check if (hasName) error() ?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:406
@@ +405,3 @@
+      } else if (Key == "type") {
+        hasKind = true;
+        if (!parseScalarString(I->getValue(), Value, Buffer))
----------------
Missing check if (hasKind) error() ?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:477
@@ +476,3 @@
+
+    if (Kind == EK_File) {
+      return new FileEntry(Name, ExternalContents);
----------------
Please switch() so that we will get a warning if we add more enumerators.

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:246
@@ +245,3 @@
+    "  'type': 'directory',\n"
+    "  'name': '/',\n"
+    "  'contents': [ {\n"
----------------
Do we check while parsing the file that the top-level entity is a directory with name '/'?

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:296
@@ +295,3 @@
+
+TEST(VirtualFileSystemTest, caseSensitive) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
----------------
Test name should probably be 'CaseInsensitive'.

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:330
@@ +329,3 @@
+
+TEST(VirtualFileSystemTest, caseInsensitive) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
----------------
Test name should probably be 'CaseSensitive'.

================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:348-349
@@ +347,4 @@
+
+  IntrusiveRefCntPtr<vfs::OverlayFileSystem>
+    O(new vfs::OverlayFileSystem(Lower));
+  O->pushOverlay(FS);
----------------
Here and in other places: when breaking a line, the second and latter lines are double-indented at least.  I am not sure if this is explicit in the style guide, but this is what clang-format does.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:254
@@ +253,3 @@
+///   <optional configuration>
+///   'roots': [
+///              <directory entries>
----------------
If we want 'roots' to model Windows drives and similar things, then we also need some kind of label for each root.


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