clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed.
After reviewing more, we should just re-use CreateBreakpoint and add a "llvm::Optional<StringRef> request_path" argument. Then all breakpoints use the same function and we avoid duplicated code. Inline comments should detail what should be done. Let me know if you have questions. ================ Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:285 // } llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp) { // Each breakpoint location is treated as a separate breakpoint for VS code. ---------------- Read my comments below first, then read this: we should add an extra llvm::Optional<StringRef> arg to this so we can have all code just use this function: ``` llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp, llvm::Optional<StringRef> request_path) ``` If "request_path" has a value, then we use this in the "source" response. If it doesn't we look it up in from the location address. ================ Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:318 object.try_emplace("line", line); object.try_emplace("source", CreateSource(line_entry)); } ---------------- we will need to add "llvm::Optional<StringRef> request_path" as an argument to CreateSource here, so it can use request_path if it has a value, else use the source file in the line_entry object. ================ Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:323 +llvm::json::Value CreateSourceBreakpoint(lldb::SBBreakpoint &bp, + llvm::StringRef sourcePath, int line) { ---------------- Actually the _only_ difference between this function and "llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp)" is the fact that we pass along the path along. So maybe we can just add a new variable to CreateBreakpoint above so. See my inline comment on CreateBreakpoint(). ================ Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:332 + + object.try_emplace("line", line); + ---------------- This should do what it was doing before: grab the source line only from the address. Why? If you set a breakpoint on a line that has no code like: ``` 1 int main(int argc, ...) 2 { /// <-- set bp here 3 int foo = arg * 10; /// <-- bp will end up here ``` So we should still be looking up the line number from the Address of the first resolved location so that the source line does get adjusted in the IDE. ================ Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:493 // } llvm::json::Value CreateSource(lldb::SBLineEntry &line_entry) { llvm::json::Object object; ---------------- add llvm::Optional<StringRef> request_path argument for CreateBreakpoint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76968/new/ https://reviews.llvm.org/D76968 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits