[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

stella.stamenova added a comment. This change broke one of the tests on Windows: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1099 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 _

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

This revision was automatically updated to reflect the committed changes. Closed by commit rL352538: [Reproducers] Add file provider (authored by JDevlieghere, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54617?vs=183968&id=184156#t

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath accepted this revision. labath added a comment. Looks good, thanks for bearing with me. I don't want to hold this up further, but ideally, I'd change that last test to use positive assertions instead of negative ones. E.g., right now this doesn't really fulfill my goal of documenting how

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere updated this revision to Diff 183968. JDevlieghere marked 4 inline comments as done. JDevlieghere added a comment. - Feedback pavel CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 Files: include/lldb/Host/FileSystem.h include/ll

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added a comment. I don't have any major issues with this patch. The thing I'd like to see though is one test which uses a path with `..` components and symlinks. If nothing else, it would serve as a good documentation for how these are supposed to be handled. Comment

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. I don't have other comments, this is good to go for me. Pavel? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere added inline comments. Comment at: source/Utility/FileCollector.cpp:27-33 + // Change path to all upper case and ask for its real path, if the latter + // exists and is equal to path, it's not case sensitive. Default to case + // sensitive in the absence of real_p

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere updated this revision to Diff 183641. JDevlieghere marked 9 inline comments as done. JDevlieghere added a comment. Code review feedback Davide. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 Files: include/lldb/Host/FileSystem.h

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

davide added a comment. Some comments. Thanks! Comment at: include/lldb/Utility/FileCollector.h:5-7 +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// You might want to update the license. =

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere updated this revision to Diff 183631. JDevlieghere added a comment. - Fix issue in lit test. - Add unit test for the file collector. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 Files: include/lldb/Host/FileSystem.h include/l

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere marked an inline comment as done. JDevlieghere added inline comments. Comment at: include/lldb/Utility/Reproducer.h:91-92 + +const char *FileInfo::name = "files"; +const char *FileInfo::file = "files.yaml"; + labath wrote: > Are you sure this can be

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added inline comments. Comment at: include/lldb/Utility/Reproducer.h:91-92 + +const char *FileInfo::name = "files"; +const char *FileInfo::file = "files.yaml"; + Are you sure this can be in a header? I would expect this to give you multiply-defined symbol

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere updated this revision to Diff 182158. JDevlieghere marked 4 inline comments as done. JDevlieghere added a comment. Feedback + Rebase on D56814 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 Files:

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added inline comments. Comment at: source/Host/common/FileSystem.cpp:48 -void FileSystem::Initialize() { +llvm::Error FileSystem::Initialize() { lldbassert(!InstanceImpl() && "Already initialized."); Would it be possible to have the overloads which al

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere planned changes to this revision. JDevlieghere added a comment. I'll add a more specific test for the collector. Comment at: source/Host/common/FileSystem.cpp:443 + + // If VFS mapped we know the underlying FS is a RedirectingFileSystem. + ErrorOr E = ---

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere updated this revision to Diff 182090. JDevlieghere marked 6 inline comments as done. JDevlieghere added a comment. Address Pavel's feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 Files: include/lldb/Host/FileSystem.h i

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added a comment. Overall, I think the patch is pretty good. I made a bunch of inline suggestions/questions/comments, but they're all fairly minor. From a high-level view the two comments I have are: - I am slightly concerned about the non-temporality of this approach. Lldb does a fair a

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

JDevlieghere added a comment. With the LLVM changes now checked-in this is ready for review. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 ___ lldb-commits mailing list ll