[YAMLVFSWriter] Fix for delimiters

[jkorous]

llvm/lib/Support/VirtualFileSystem.cpp

2065–2066

I feel this delimiter should also be conditional on IsCurrentDirEmpty but need to think about it a bit more.

I'll add a testcase for entries like this:

/a/
/a/b/

[jkorous]

We also need to put a delimiter conditionally whenever we are changing the "current directory" (represtented by DirStack.back()).
Basically there are two cases:

1. We are popping from DirStack - that means there's always some previous directory entry.

E. g. the transition from

a [
 b [ ]
]

to

a [
 b [ ], <--- this is the delimiter
 c [ ]
]

2. We are adding a subdirectory to existing directory with already existing entries.

E. g. the transition from

a [
 b.txt
]

to

a [
 b.txt, <--- this is the delimiter
 c [ ]
]

[jkorous]

llvm/lib/Support/VirtualFileSystem.cpp

2065–2066

Hopefully now addressed.

[JDevlieghere]

Thank you for fixing this Jan. I ran the LLDB reproducer test suite and everything passes.

llvm/lib/Support/VirtualFileSystem.cpp

2043

So this is set to true whenever we start a new directory, and is set to false whenever we write a file entry. Would it make sense to make this variable a member and toggle it inside startDirectory and writeEntry respectively?

2057

Not a fan of this name. What about IsDirPoppedFromStack?

[jkorous]

llvm/lib/Support/VirtualFileSystem.cpp

2043

Maybe... I'd assume that if we just reset it at the start of write() method it should work.

That said - I kind of prefer to keep the whole lifecycle of the variable visible in this method as it's imo easier to understand than having to check two other methods (no matter how trivial).
Also, although I wouldn't expect any complications by doing that (can't imagine any potential misuse of the class interface or possibility to corrupt instance state) but I'd still prefer to land this patch as it is and address this maybe later.

2057

Sounds good

[jkorous]

Renamed local var

llvm/lib/Support/VirtualFileSystem.cpp

if (IsOverlayRelative.hasValue()) {
     UseOverlayRelative = IsOverlayRelative.getValue();
     OS << "  'overlay-relative': '" << (UseOverlayRelative ? "true" : "false")
        << "',\n";
   }
   OS << "  'roots': [\n";
 
   if (!Entries.empty()) {
     const YAMLVFSEntry &Entry = Entries.front();
-    bool first_entry_is_directory = Entry.IsDirectory;
-    StringRef Dir =
-        first_entry_is_directory ? Entry.VPath : path::parent_path(Entry.VPath);
-    startDirectory(Dir);
+
+    startDirectory(
+      Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath)
+    );
 
     StringRef RPath = Entry.RPath;
     if (UseOverlayRelative) {
       unsigned OverlayDirLen = OverlayDir.size();
       assert(RPath.substr(0, OverlayDirLen) == OverlayDir &&
              "Overlay dir must be contained in RPath");
       RPath = RPath.slice(OverlayDirLen, RPath.size());
     }
 
-    if (!first_entry_is_directory)
+    bool IsCurrentDirEmpty = true;
+    if (!Entry.IsDirectory) {
       writeEntry(path::filename(Entry.VPath), RPath);
+      IsCurrentDirEmpty = false;
+    }
 
     for (const auto &Entry : Entries.slice(1)) {
       StringRef Dir =
           Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath);
       if (Dir == DirStack.back()) {
-        if (!first_entry_is_directory) {
+        if (!IsCurrentDirEmpty) {
           OS << ",\n";
-          first_entry_is_directory = false;
         }
       } else {
+        bool HaveWePoppedDirStack = false;
         while (!DirStack.empty() && !containedIn(DirStack.back(), Dir)) {
           OS << "\n";
           endDirectory();
+          HaveWePoppedDirStack = true;
         }
+        if (HaveWePoppedDirStack || !IsCurrentDirEmpty) {
           OS << ",\n";
+        }
         startDirectory(Dir);
+        IsCurrentDirEmpty = true;
       }
       StringRef RPath = Entry.RPath;
       if (UseOverlayRelative) {
         unsigned OverlayDirLen = OverlayDir.size();
         assert(RPath.substr(0, OverlayDirLen) == OverlayDir &&
                "Overlay dir must be contained in RPath");
         RPath = RPath.slice(OverlayDirLen, RPath.size());
       }
-      if (!Entry.IsDirectory)
+      if (!Entry.IsDirectory) {
         writeEntry(path::filename(Entry.VPath), RPath);
+        IsCurrentDirEmpty = false;
+      }
     }
 
     while (!DirStack.empty()) {
       OS << "\n";
       endDirectory();
     }
     OS << "\n";
   }

llvm/unittests/Support/VirtualFileSystemTest.cpp

TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest2) {
 
   std::string Buffer;
   raw_string_ostream OS(Buffer);
   VFSWriter.write(OS);
   OS.flush();
 
   IntrusiveRefCntPtr<ErrorDummyFileSystem> Lower(new ErrorDummyFileSystem());
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLRawString(Buffer, Lower);
-  // FIXME: Missing comma separator between file entries.
-  EXPECT_FALSE(FS.get() != nullptr);
+  EXPECT_TRUE(FS.get() != nullptr);
 }
 
 TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest3) {
   ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
   ScopedDir _a(TestDirectory + "/a");
   ScopedFile _ab(TestDirectory + "/a/b", "");
   ScopedDir _ac(TestDirectory + "/a/c");
   ScopedDir _acd(TestDirectory + "/a/c/d");
TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest3) {
 
   std::string Buffer;
   raw_string_ostream OS(Buffer);
   VFSWriter.write(OS);
   OS.flush();
 
   IntrusiveRefCntPtr<ErrorDummyFileSystem> Lower(new ErrorDummyFileSystem());
   IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLRawString(Buffer, Lower);
-  // FIXME: Spurious comma separator before first file entry in directory.
-  EXPECT_FALSE(FS.get() != nullptr);
+  EXPECT_TRUE(FS.get() != nullptr);
 }
 
 TEST_F(VFSFromYAMLTest, YAMLVFSWriterTestHandleDirs) {
   ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true);
   ScopedDir _a(TestDirectory + "/a");
   ScopedDir _b(TestDirectory + "/b");
   ScopedDir _c(TestDirectory + "/c");