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