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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
