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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
