[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked an inline comment as done.
bnbarham added a comment.

Merged into D121425  instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked an inline comment as done.
bnbarham added inline comments.



Comment at: llvm/include/llvm/Support/Error.h:1284
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 

dexonsmith wrote:
> bnbarham wrote:
> > Should this be in a change all by itself?
> Yes, but I also know you already split this out so I guess you just need to 
> rebase :).
Heh, when you suggested I pull the rename out I basically thought "welll, that 
answer this question"

So yep, it's out and I'll rebase this one :). Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

In D121426#3376442 , @dexonsmith 
wrote:

>> Includes two test fixes (since chained mappings are no longer allowed)
>> and adds a new test for multiple overlays.
>
> Are we sure no one needs to support chained mappings? Has there been a 
> ~~clang-dev~~ discourse discussion about it already? Just concerned that some 
> vendor might rely on being able to support this.

I'm not *positive*, no, but I would be fairly surprised. You could just add the 
`A -> C` mapping if you really do want it. But I can start up that conversation 
if you think it needs having.

I actually didn't initially realise that there was a test for this case - 
`vfsroot-with-overlay.c` did test "indirection", but I completely missed it 
when I was looking through. I *thought* the only case was the one Nathan added 
in `directory.c` (and in that case what we really wanted was was what's now 
`fallback`).

@vsapsai do you know any clients of the chaining/nesting/indirection?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

> Includes two test fixes (since chained mappings are no longer allowed)
> and adds a new test for multiple overlays.

Are we sure no one needs to support chained mappings? Has there been a 
~~clang-dev~~ discourse discussion about it already? Just concerned that some 
vendor might rely on being able to support this.




Comment at: llvm/include/llvm/Support/Error.h:1284
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 

bnbarham wrote:
> Should this be in a change all by itself?
Yes, but I also know you already split this out so I guess you just need to 
rebase :).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham updated this revision to Diff 414751.
bnbarham edited the summary of this revision.
bnbarham added a comment.

Update to single review dependency


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// this instance remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code );
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code ) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase ) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
 
 #include "not_real.h"
Index: clang/test/VFS/multiple-overlays.c
===
--- /dev/null
+++ clang/test/VFS/multiple-overlays.c
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" %t/vfs/base.yaml > %t/vfs/a-b.yaml
+// RUN: 

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham updated this revision to Diff 414718.
bnbarham added a comment.

Handle empty overlay file in clang tidy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// this instance remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code );
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code ) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase ) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
 
 #include "not_real.h"
Index: clang/test/VFS/multiple-overlays.c
===
--- /dev/null
+++ clang/test/VFS/multiple-overlays.c
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" %t/vfs/base.yaml > %t/vfs/a-b.yaml
+// RUN: sed -e 

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: llvm/include/llvm/Support/Error.h:1284
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 

Should this be in a change all by itself?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121426/new/

https://reviews.llvm.org/D121426

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham created this revision.
bnbarham added reviewers: keith, dexonsmith, JDevlieghere, vsapsai.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.
bnbarham requested review of this revision.
Herald added projects: clang, LLDB, LLVM, clang-tools-extra.
Herald added subscribers: cfe-commits, llvm-commits, lldb-commits.

Includes two test fixes (since chained mappings are no longer allowed)
and adds a new test for multiple overlays.

Uses other than `CompilerInvocation.cpp` are simple 1:1 mappings, but
without the need to read into a buffer first.

Depends on D121421  and D121423 
 and D121424 
 and D121425 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code );
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code ) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase ) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_cc1