This is an archive of the discontinued LLVM Phabricator instance.

[YAMLVFSWriter] Fix for delimiters
ClosedPublic

Authored by jkorous on May 12 2020, 2:49 PM.

Diff Detail

Event Timeline

jkorous created this revision.May 12 2020, 2:49 PM
jkorous marked an inline comment as done.May 12 2020, 2:52 PM
jkorous added inline comments.
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 updated this revision to Diff 263526.May 12 2020, 3:15 PM
jkorous retitled this revision from [YAMLVFSWriter] Speculative fix for delimiters to [YAMLVFSWriter] Fix for delimiters.

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 [ ]
]
  1. 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 marked an inline comment as done.May 12 2020, 3:16 PM
jkorous added inline comments.
llvm/lib/Support/VirtualFileSystem.cpp
2065–2066

Hopefully now addressed.

JDevlieghere accepted this revision.May 12 2020, 3:26 PM

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?

This revision is now accepted and ready to land.May 12 2020, 3:26 PM
jkorous marked 3 inline comments as done.May 12 2020, 3:38 PM
jkorous added inline comments.
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 updated this revision to Diff 263547.May 12 2020, 3:40 PM
jkorous marked an inline comment as done.

Renamed local var

This revision was automatically updated to reflect the committed changes.