r250036 - [VFS] Don't try to be heroic with '.' in paths.

[Manuel Klimek via cfe-commits]

I believe we need a more principled approach here :)

1. The first thing we need is to actually produce nice errors when we hit
an addFile() with the same path (normalized or non-normalized)
I'd propose the following approach: we compare the buffers; if the buffer
contents are equal, we declare success; otherwise we return a nice
descriptive error ("trying to add the same file with different content").

2. I think we'll want 2 different modes in InMemoryFileSystem: Normalize
should be either on or off; if on, we should normalize everything, and
return an error if we hop up over /. if off, we should not normalize
anything.

On Mon, Oct 12, 2015 at 3:32 PM Benjamin Kramer via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: d0k
> Date: Mon Oct 12 08:30:38 2015
> New Revision: 250036
>
> URL: http://llvm.org/viewvc/llvm-project?rev=250036&view=rev
> Log:
> [VFS] Don't try to be heroic with '.' in paths.
>
> Actually the only special path we have to handle is ./foo, the rest is
> tricky to get right so do the same thing as the existing YAML vfs here.
>
> Modified:
>     cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>     cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
>
> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=250036&r1=250035&r2=250036&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Mon Oct 12 08:30:38 2015
> @@ -10,7 +10,6 @@
>
>  //===----------------------------------------------------------------------===//
>
>  #include "clang/Basic/VirtualFileSystem.h"
> -#include "clang/Basic/FileManager.h"
>  #include "llvm/ADT/DenseMap.h"
>  #include "llvm/ADT/STLExtras.h"
>  #include "llvm/ADT/StringExtras.h"
> @@ -497,12 +496,14 @@ void InMemoryFileSystem::addFile(const T
>    assert(!EC);
>    (void)EC;
>
> -  FileManager::removeDotPaths(Path, /*RemoveDotDot=*/false);
> -  if (Path.empty())
> -    return;
> -
>    detail::InMemoryDirectory *Dir = Root.get();
>    auto I = llvm::sys::path::begin(Path), E = llvm::sys::path::end(Path);
> +  if (*I == ".")
> +    ++I;
> +
> +  if (I == E)
> +    return;
> +
>    while (true) {
>      StringRef Name = *I;
>      detail::InMemoryNode *Node = Dir->getChild(Name);
> @@ -556,11 +557,13 @@ lookupInMemoryNode(const InMemoryFileSys
>    assert(!EC);
>    (void)EC;
>
> -  FileManager::removeDotPaths(Path, /*RemoveDotDot=*/false);
> -  if (Path.empty())
> +  auto I = llvm::sys::path::begin(Path), E = llvm::sys::path::end(Path);
> +  if (*I == ".")
> +    ++I;
> +
> +  if (I == E)
>      return Dir;
>
> -  auto I = llvm::sys::path::begin(Path), E = llvm::sys::path::end(Path);
>    while (true) {
>      detail::InMemoryNode *Node = Dir->getChild(*I);
>      ++I;
>
> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=250036&r1=250035&r2=250036&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Mon Oct 12
> 08:30:38 2015
> @@ -551,8 +551,6 @@ TEST_F(InMemoryFileSystemTest, OverlayFi
>    FS.addFile("/a", 0, MemoryBuffer::getMemBuffer("a"));
>    auto Stat = FS.status("/");
>    ASSERT_FALSE(Stat.getError()) << Stat.getError() << FS.toString();
> -  Stat = FS.status("/.");
> -  ASSERT_FALSE(Stat.getError()) << Stat.getError() << FS.toString();
>    Stat = FS.status("/a");
>    ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" <<
> FS.toString();
>    ASSERT_EQ("/a", Stat->getName());
> @@ -568,18 +566,18 @@ TEST_F(InMemoryFileSystemTest, OverlayFi
>
>  TEST_F(InMemoryFileSystemTest, OpenFileForRead) {
>    FS.addFile("/a", 0, MemoryBuffer::getMemBuffer("a"));
> -  FS.addFile("././c", 0, MemoryBuffer::getMemBuffer("c"));
> +  FS.addFile("./c", 0, MemoryBuffer::getMemBuffer("c"));
>    auto File = FS.openFileForRead("/a");
>    ASSERT_EQ("a", (*(*File)->getBuffer("ignored"))->getBuffer());
>    File = FS.openFileForRead("/a"); // Open again.
>    ASSERT_EQ("a", (*(*File)->getBuffer("ignored"))->getBuffer());
> -  File = FS.openFileForRead("/././a"); // Open again.
> +  File = FS.openFileForRead("./a"); // Open again.
>    ASSERT_EQ("a", (*(*File)->getBuffer("ignored"))->getBuffer());
>    File = FS.openFileForRead("/");
>    ASSERT_EQ(File.getError(), errc::invalid_argument) << FS.toString();
>    File = FS.openFileForRead("/b");
>    ASSERT_EQ(File.getError(), errc::no_such_file_or_directory) <<
> FS.toString();
> -  File = FS.openFileForRead("./c");
> +  File = FS.openFileForRead("c");
>    ASSERT_EQ("c", (*(*File)->getBuffer("ignored"))->getBuffer());
>  }
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151012/998bb2c6/attachment.html>