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

[Dmitri Gribenko]

  I think it is better to move the YAML VFS description to a new file under 'docs/'.  This file format is API, and should be documented.

  Unrelated to this patch, but how do we keep track of the current working directory?  Or we don't just yet?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:205
@@ +204,3 @@
+
+/// \brief Represents the value of a 'type' field.
+enum EntryKind {
----------------
This comment is not really helpful...

================
Comment at: lib/Basic/VirtualFileSystem.cpp:252-257
@@ +251,8 @@
+/// The basic structure of the parsed file is:
+/// {
+///   <optional configuration>
+///   'roots': [
+///              <directory entries>
+///            ]
+/// }
+///
----------------
Please use \verbatim ... \endverbatim.  Doxygen HTML output looks better that way.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:260
@@ +259,3 @@
+/// Possible configuration settings are
+///   'case-sensitive': <boolean>
+///
----------------
Is it optional?  If so, what is the default?

Please also add a mandatory version field, and refuse to load files with incorrect version.  I hope we will never have to bump the version and evolve the format in a compatible way, but who knows...

================
Comment at: lib/Basic/VirtualFileSystem.cpp:280
@@ +279,3 @@
+///   'name': <string>,
+///   'external-contents': <path to external file>)
+/// }
----------------
Stray ')'

While in file format 'external-contents' looks unambiguous, in source code
  StringRef getExternalContents()
and similar are not completely clear: does it return a path or the actual contents?  Maybe rename all similar variables and functions in source code to *ExternalContentsPath*?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:290
@@ +289,3 @@
+  /// \brief The file system to use for 'external-contents'.
+  IntrusiveRefCntPtr<FileSystem> ExternalFS; ///< The file system to use for
+
----------------
... to use for *external references*?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:302-303
@@ +301,4 @@
+      : ExternalFS(ExternalFS) {
+    llvm::Triple Triple(LLVM_HOST_TRIPLE);
+    CaseSensitive = Triple.isOSDarwin() || Triple.isOSWindows();
+  }
----------------
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.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:306-307
@@ +305,4 @@
+
+  /// \brief Looks up \p Path in \p Roots.
+  ErrorOr<Entry *> lookupPath(const Twine &Path);
+
----------------
Nitpick: it is not semantically correct to refer to Roots as \p, because it is not a parameter.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:321
@@ +320,3 @@
+  /// returns a virtual file system representing its contents.
+  static VFSFromYAML *Create(MemoryBuffer *Buffer,
+                             SourceMgr::DiagHandlerTy DiagHandler,
----------------
Function names start with a lowercase letter.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:330-331
@@ +329,4 @@
+
+// A helper class to hold the common YAML parsing state.
+class VFSFromYAMLParser {
+  yaml::Stream &Stream;
----------------
Three slashes?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:542-554
@@ +541,15 @@
+Entry::~Entry() {}
+DirectoryEntry::~DirectoryEntry() {
+  for (std::vector<Entry *>::iterator I = Contents.begin(), E = Contents.end();
+       I != E; ++I) {
+    delete *I;
+  }
+}
+
+VFSFromYAML::~VFSFromYAML() {
+  for (std::vector<Entry *>::iterator I = Roots.begin(), E =Roots.end(); I != E;
+       ++I) {
+    delete *I;
+  }
+}
+
----------------
llvm::DeleteContainerPointers?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:582
@@ +581,3 @@
+
+error_code normalizePath(StringRef Path, SmallVectorImpl<char> &Result) {
+  sys::path::const_iterator I = sys::path::begin(Path);
----------------
static?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:583-584
@@ +582,4 @@
+error_code normalizePath(StringRef Path, SmallVectorImpl<char> &Result) {
+  sys::path::const_iterator I = sys::path::begin(Path);
+  sys::path::const_iterator End = sys::path::end(Path);
+
----------------
Please move these iterators to the for loop so that there is no confusion about their values being significant after the loop exits.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:591-595
@@ +590,7 @@
+    if (I->equals("..")) {
+      if (Components.empty())
+        return error_code(errc::invalid_argument, system_category());
+      Components.pop_back();
+      if (Components.empty())
+        return error_code(errc::invalid_argument, system_category());
+      continue;
----------------
Why error here?  Maybe there is some precondition about 'Path' that is not documented?

/../../../ is /, because / has a '..' entry that points back to itself.

What about paths relative to current dir?  Should not be passed here?

Also, I don't think we can actually normalize the path without looking at the filesystem.  normalize_path("/zzz/..") -> "/", but this is not how OS works:

  $ ls /zzz/..
  ls: /zzz/..: No such file or directory



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