[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Many thanks! Great cleanup. Just a few nits and we're good to go



================
Comment at: lib/Basic/VirtualFileSystem.cpp:475
   InMemoryNodeKind Kind;
+  Status Stat;
+
----------------
NIT: maybe keep the order of members the same to keep the patch more focused? Unless there's a good reason to swap them.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:528
   InMemoryFile &Node;
 
+  /// The name to use when returning a Status for this file.
----------------
NIT: remove this blank line to follow the code style of the file more closely?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:770
+      llvm::sys::path::append(Path, I->second->getFileName());
+      CurrentEntry = I->second->getStatus(Path.str());
+    } else {
----------------
NIT: `Path.str()` can be replaced with `Path` (SmallString is convertible to StringRef)


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:155
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
----------------
Maybe replace lambda with a funciton?


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:155
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
----------------
ilya-biryukov wrote:
> Maybe replace lambda with a funciton?
Could the name mention the expected result? E.g. `getUnixPath()` or something similar


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:156
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
+  return S;
----------------
Maybe use `llvm::sys::path::native(S, style::posix)`?


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:790
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
   I.increment(EC);
----------------
Maybe add a comment about windows and the path we get there?


Repository:
  rC Clang

https://reviews.llvm.org/D48903