[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol
jkorous added a comment.
Hi Greg, this looks interesting! I got curious about your patch since I am
dealing with another protocol from the same "family" - LSP.
Comment at: tools/lldb-vscode/lldb-vscode.cpp:1424
+//--
+std::string read_json_packet(FILE *in) {
+ static std::string header("Content-Length: ");
It looks to me that since you are just logging protocol level error here they
will manifest as JSON parsing errors down the road (L 4113). Is that ok?
BTW there's another implementation of HTTP-like header parsing in clangd:
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/JSONRPCDispatcher.cpp#L238
Comment at: tools/lldb-vscode/lldb-vscode.cpp:4107
+ uint32_t packet_idx = 0;
+ while (true) {//!feof(g_state.in) && !ferror(g_state.in)) {
+std::string json = read_json_packet(g_state.in);
This looks like some possible forgotten debugging artefact.
https://reviews.llvm.org/D50365
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D65237: [Support] move FileCollector from LLDB to llvm/Support
jkorous added inline comments. Comment at: llvm/include/llvm/Support/FileCollector.h:40 + +protected: + void AddFileImpl(StringRef src_path); TLDR: private? I'm just wondering if we could make the class safer or the correct use more obvious for classes deriving from it (if we want to support that). The couple protected members and methods seem suggesting it's fine to use these from a derived class implementation. IIUC `m_mutex` is guarding `m_vfs_writer`, `m_seen ` and `m_symlink_map` and while it is being locked in implementation of public method `AddFile`, protected methods seem to be assuming their callers lock the mutex. Specifically there's a potential for a data race in `AddFileImpl` (calling `GetRealPath` which uses `m_symlink_map`) and in `AddFileToMapping` if either is called directly from a derived class. Since `lldb_private::FileCollector` seems to be using only the public interface we might just declare the above private (and maybe add a doc comment when caller of a method is expected to lock the mutex)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65237/new/ https://reviews.llvm.org/D65237 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure
jkorous added inline comments. Comment at: clang/include/clang/Basic/FileManager.h:143 /// - llvm::StringMap SeenDirEntries; + llvm::StringMap, llvm::BumpPtrAllocator> + SeenDirEntries; Maybe we could replace this with some type that has hard-to-use-incorrectly interface instead of using `containsValue()`. Comment at: clang/include/clang/Basic/FileManager.h:217 /// - /// This returns NULL if the file doesn't exist. + /// This returns a \c std::error_code if there was an error loading the file. /// Does that mean that it's now safe to assume the value is `!= NULL` in the absence of errors? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65534/new/ https://reviews.llvm.org/D65534 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure
jkorous added inline comments.
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:240
+auto File = SourceMgr.getFileManager().getFile(FilePath);
+if (!File)
+ return SourceLocation();
Previously we'd hit the assert in `translateFile()` called from
`getOrCreateFileID()`. This seems like a better way to handle that.
Comment at: clang/include/clang/Basic/FileManager.h:217
///
- /// This returns NULL if the file doesn't exist.
+ /// This returns a \c std::error_code if there was an error loading the file.
///
harlanhaskins wrote:
> JDevlieghere wrote:
> > harlanhaskins wrote:
> > > jkorous wrote:
> > > > Does that mean that it's now safe to assume the value is `!= NULL` in
> > > > the absence of errors?
> > > That's the intent of these changes, yes, but it should also be
> > > documented. 👍
> > In line with the previous comment, should we just pass a reference then?
> I'm fine with doing that, but it would introduce a significant amount more
> churn into this patch.
I feel that this patch is already huge. It's a good idea though - maybe a
separate patch?
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+ auto newE = FileMgr->getFile(tempPath);
+ if (newE) {
+remap(origFE, *newE);
It seems like we're now avoiding the assert in `remap()` we'd hit previously.
Is this fine?
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:229
const FileEntry *FileRemapper::getOriginalFile(StringRef filePath) {
- const FileEntry *file = FileMgr->getFile(filePath);
+ const FileEntry *file;
+ if (auto fileOrErr = FileMgr->getFile(filePath))
Shouldn't we initialize this guy to `nullptr`?
Comment at: clang/lib/Basic/FileManager.cpp:73
if (llvm::sys::path::is_separator(Filename[Filename.size() - 1]))
-return nullptr; // If Filename is a directory.
+return std::errc::is_a_directory; // If Filename is a directory.
I think you've made the code self-documenting. Could you please remove the
comment?
Comment at: clang/lib/Frontend/FrontendAction.cpp:715
SmallString<128> DirNative;
- llvm::sys::path::native(PCHDir->getName(), DirNative);
+ if (*PCHDir == nullptr) {
+llvm::dbgs() << "Directory " << PCHInclude << " was null but no error
thrown";
Could this actually happen? I was expecting the behavior to be consistent with
`getFile()`.
Comment at: clang/lib/Lex/HeaderSearch.cpp:1448
if (getHeaderSearchOpts().ModuleMapFileHomeIsCwd)
-Dir = FileMgr.getDirectory(".");
+Dir = *FileMgr.getDirectory(".");
else {
Seems like we're introducing assert here. Is that fine?
Comment at: clang/unittests/Basic/FileManagerTest.cpp:260
+
+ EXPECT_EQ(f1 ? *f1 : nullptr,
+f2 ? *f2 : nullptr);
Just an idea - should we compare error codes?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65534/new/
https://reviews.llvm.org/D65534
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure
jkorous accepted this revision.
jkorous marked an inline comment as done.
jkorous added a comment.
This revision is now accepted and ready to land.
LGTM
Thanks for all the work here!
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+ auto newE = FileMgr->getFile(tempPath);
+ if (newE) {
+remap(origFE, *newE);
harlanhaskins wrote:
> jkorous wrote:
> > It seems like we're now avoiding the assert in `remap()` we'd hit
> > previously. Is this fine?
> If we want to trap if this temp file fails, then I'm happy removing the
> condition check. Do you think this should trap on failure or should it check
> the condition?
To be clear - I didn't have any opinion, just noticed there's a functional
change and pointed that out.
On the other hand I assume your solution is fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65534/new/
https://reviews.llvm.org/D65534
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
