[lldb-dap] Refactoring breakpoints to not use the g_dap reference.
#115208
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesRefactoring breakpoints to not use the Instead, when a breakpoint is constructed it will be passed a DAP reference that it should use for its lifetime. This is part of a larger refactor to remove the global Patch is 47.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115208.diff 19 Files Affected:
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 9ea7a42ca85a1e..aee2f87e2cc23e 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -7,11 +7,13 @@
//===----------------------------------------------------------------------===//
#include "Breakpoint.h"
-#include "DAP.h"
-#include "JSONUtils.h"
+
#include "lldb/API/SBBreakpointLocation.h"
#include "llvm/ADT/StringExtras.h"
+#include "DAP.h"
+#include "JSONUtils.h"
+
using namespace lldb_dap;
void Breakpoint::SetCondition() { bp.SetCondition(condition.c_str()); }
@@ -51,7 +53,7 @@ void Breakpoint::CreateJsonObject(llvm::json::Object &object) {
if (bp_addr.IsValid()) {
std::string formatted_addr =
- "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(g_dap.target));
+ "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(bp.GetTarget()));
object.try_emplace("instructionReference", formatted_addr);
auto line_entry = bp_addr.GetLineEntry();
const auto line = line_entry.GetLine();
diff --git a/lldb/tools/lldb-dap/Breakpoint.h b/lldb/tools/lldb-dap/Breakpoint.h
index ee9d3736d6190f..cffeb2fab1f0ef 100644
--- a/lldb/tools/lldb-dap/Breakpoint.h
+++ b/lldb/tools/lldb-dap/Breakpoint.h
@@ -9,18 +9,19 @@
#ifndef LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
#define LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
-#include "BreakpointBase.h"
#include "lldb/API/SBBreakpoint.h"
+#include "BreakpointBase.h"
+
namespace lldb_dap {
struct Breakpoint : public BreakpointBase {
// The LLDB breakpoint associated wit this source breakpoint
lldb::SBBreakpoint bp;
- Breakpoint() = default;
- Breakpoint(const llvm::json::Object &obj) : BreakpointBase(obj){};
- Breakpoint(lldb::SBBreakpoint bp) : bp(bp) {}
+ Breakpoint(DAP &d) : BreakpointBase(d) {}
+ Breakpoint(DAP &d, const llvm::json::Object &obj) : BreakpointBase(d, obj) {}
+ Breakpoint(DAP &d, lldb::SBBreakpoint bp) : BreakpointBase(d), bp(bp) {}
void SetCondition() override;
void SetHitCondition() override;
diff --git a/lldb/tools/lldb-dap/BreakpointBase.cpp b/lldb/tools/lldb-dap/BreakpointBase.cpp
index f3cb06a3562d48..c5d7a9778df018 100644
--- a/lldb/tools/lldb-dap/BreakpointBase.cpp
+++ b/lldb/tools/lldb-dap/BreakpointBase.cpp
@@ -11,8 +11,8 @@
using namespace lldb_dap;
-BreakpointBase::BreakpointBase(const llvm::json::Object &obj)
- : condition(std::string(GetString(obj, "condition"))),
+BreakpointBase::BreakpointBase(DAP &d, const llvm::json::Object &obj)
+ : dap(d), condition(std::string(GetString(obj, "condition"))),
hitCondition(std::string(GetString(obj, "hitCondition"))) {}
void BreakpointBase::UpdateBreakpoint(const BreakpointBase &request_bp) {
diff --git a/lldb/tools/lldb-dap/BreakpointBase.h b/lldb/tools/lldb-dap/BreakpointBase.h
index 79301480e0e588..bb660ddc451bd5 100644
--- a/lldb/tools/lldb-dap/BreakpointBase.h
+++ b/lldb/tools/lldb-dap/BreakpointBase.h
@@ -9,12 +9,17 @@
#ifndef LLDB_TOOLS_LLDB_DAP_BREAKPOINTBASE_H
#define LLDB_TOOLS_LLDB_DAP_BREAKPOINTBASE_H
-#include "llvm/Support/JSON.h"
#include <string>
+#include "llvm/Support/JSON.h"
+
+#include "DAPForward.h"
+
namespace lldb_dap {
struct BreakpointBase {
+ // Associated DAP session.
+ DAP &dap;
// An optional expression for conditional breakpoints.
std::string condition;
@@ -22,8 +27,9 @@ struct BreakpointBase {
// ignored. The backend is expected to interpret the expression as needed
std::string hitCondition;
- BreakpointBase() = default;
- BreakpointBase(const llvm::json::Object &obj);
+ BreakpointBase(DAP &d) : dap(d) {}
+ BreakpointBase(DAP &d, const llvm::json::Object &obj);
+ BreakpointBase(const BreakpointBase &) = default;
virtual ~BreakpointBase() = default;
virtual void SetCondition() = 0;
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 283392270ba26c..9921e15e1e75cd 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -74,21 +74,21 @@ void DAP::PopulateExceptionBreakpoints() {
exception_breakpoints = std::vector<ExceptionBreakpoint>{};
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeC_plus_plus)) {
- exception_breakpoints->emplace_back("cpp_catch", "C++ Catch",
+ exception_breakpoints->emplace_back(*this, "cpp_catch", "C++ Catch",
lldb::eLanguageTypeC_plus_plus);
- exception_breakpoints->emplace_back("cpp_throw", "C++ Throw",
+ exception_breakpoints->emplace_back(*this, "cpp_throw", "C++ Throw",
lldb::eLanguageTypeC_plus_plus);
}
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeObjC)) {
- exception_breakpoints->emplace_back("objc_catch", "Objective-C Catch",
+ exception_breakpoints->emplace_back(*this, "objc_catch", "Objective-C Catch",
lldb::eLanguageTypeObjC);
- exception_breakpoints->emplace_back("objc_throw", "Objective-C Throw",
+ exception_breakpoints->emplace_back(*this, "objc_throw", "Objective-C Throw",
lldb::eLanguageTypeObjC);
}
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeSwift)) {
- exception_breakpoints->emplace_back("swift_catch", "Swift Catch",
+ exception_breakpoints->emplace_back(*this, "swift_catch", "Swift Catch",
lldb::eLanguageTypeSwift);
- exception_breakpoints->emplace_back("swift_throw", "Swift Throw",
+ exception_breakpoints->emplace_back(*this, "swift_throw", "Swift Throw",
lldb::eLanguageTypeSwift);
}
// Besides handling the hardcoded list of languages from above, we try to
@@ -118,7 +118,7 @@ void DAP::PopulateExceptionBreakpoints() {
std::string throw_keyword =
raw_throw_keyword ? raw_throw_keyword : "throw";
- exception_breakpoints->emplace_back(
+ exception_breakpoints->emplace_back(*this,
raw_lang_name + "_" + throw_keyword,
capitalized_lang_name + " " + capitalize(throw_keyword), lang);
}
@@ -129,7 +129,7 @@ void DAP::PopulateExceptionBreakpoints() {
std::string catch_keyword =
raw_catch_keyword ? raw_catch_keyword : "catch";
- exception_breakpoints->emplace_back(
+ exception_breakpoints->emplace_back(*this,
raw_lang_name + "_" + catch_keyword,
capitalized_lang_name + " " + capitalize(catch_keyword), lang);
}
@@ -1060,7 +1060,7 @@ void DAP::SetThreadFormat(llvm::StringRef format) {
InstructionBreakpoint *
DAP::GetInstructionBreakpoint(const lldb::break_id_t bp_id) {
for (auto &bp : instruction_breakpoints) {
- if (bp.second.id == bp_id)
+ if (bp.second.bp.GetID() == bp_id)
return &bp.second;
}
return nullptr;
diff --git a/lldb/tools/lldb-dap/DAPForward.h b/lldb/tools/lldb-dap/DAPForward.h
index 159d999a63c820..2bb54d806fd6fb 100644
--- a/lldb/tools/lldb-dap/DAPForward.h
+++ b/lldb/tools/lldb-dap/DAPForward.h
@@ -16,6 +16,7 @@ struct FunctionBreakpoint;
struct SourceBreakpoint;
struct Watchpoint;
struct InstructionBreakpoint;
+struct DAP;
} // namespace lldb_dap
namespace lldb {
diff --git a/lldb/tools/lldb-dap/ExceptionBreakpoint.cpp b/lldb/tools/lldb-dap/ExceptionBreakpoint.cpp
index 130c237e65441d..e9bb11face49df 100644
--- a/lldb/tools/lldb-dap/ExceptionBreakpoint.cpp
+++ b/lldb/tools/lldb-dap/ExceptionBreakpoint.cpp
@@ -17,8 +17,8 @@ void ExceptionBreakpoint::SetBreakpoint() {
return;
bool catch_value = filter.find("_catch") != std::string::npos;
bool throw_value = filter.find("_throw") != std::string::npos;
- bp = g_dap.target.BreakpointCreateForException(language, catch_value,
- throw_value);
+ bp = dap.target.BreakpointCreateForException(language, catch_value,
+ throw_value);
// See comments in BreakpointBase::GetBreakpointLabel() for details of why
// we add a label to our breakpoints.
bp.AddName(BreakpointBase::GetBreakpointLabel());
@@ -27,7 +27,7 @@ void ExceptionBreakpoint::SetBreakpoint() {
void ExceptionBreakpoint::ClearBreakpoint() {
if (!bp.IsValid())
return;
- g_dap.target.BreakpointDelete(bp.GetID());
+ dap.target.BreakpointDelete(bp.GetID());
bp = lldb::SBBreakpoint();
}
diff --git a/lldb/tools/lldb-dap/ExceptionBreakpoint.h b/lldb/tools/lldb-dap/ExceptionBreakpoint.h
index 7b81d845cb26be..7819bea726a1d0 100644
--- a/lldb/tools/lldb-dap/ExceptionBreakpoint.h
+++ b/lldb/tools/lldb-dap/ExceptionBreakpoint.h
@@ -13,17 +13,21 @@
#include "lldb/API/SBBreakpoint.h"
+#include "DAPForward.h"
+
namespace lldb_dap {
struct ExceptionBreakpoint {
+ DAP &dap;
std::string filter;
std::string label;
lldb::LanguageType language;
- bool default_value;
+ bool default_value = false;
lldb::SBBreakpoint bp;
- ExceptionBreakpoint(std::string f, std::string l, lldb::LanguageType lang)
- : filter(std::move(f)), label(std::move(l)), language(lang),
- default_value(false), bp() {}
+ ExceptionBreakpoint(DAP &d, std::string f, std::string l,
+ lldb::LanguageType lang)
+ : dap(d), filter(std::move(f)), label(std::move(l)), language(lang),
+ bp() {}
void SetBreakpoint();
void ClearBreakpoint();
diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
index 216c685f633da8..ef6df6c0dc91cc 100644
--- a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
+++ b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
@@ -7,18 +7,19 @@
//===----------------------------------------------------------------------===//
#include "FunctionBreakpoint.h"
+
#include "DAP.h"
#include "JSONUtils.h"
namespace lldb_dap {
-FunctionBreakpoint::FunctionBreakpoint(const llvm::json::Object &obj)
- : Breakpoint(obj), functionName(std::string(GetString(obj, "name"))) {}
+FunctionBreakpoint::FunctionBreakpoint(DAP &d, const llvm::json::Object &obj)
+ : Breakpoint(d, obj), functionName(std::string(GetString(obj, "name"))) {}
void FunctionBreakpoint::SetBreakpoint() {
if (functionName.empty())
return;
- bp = g_dap.target.BreakpointCreateByName(functionName.c_str());
+ bp = dap.target.BreakpointCreateByName(functionName.c_str());
Breakpoint::SetBreakpoint();
}
diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.h b/lldb/tools/lldb-dap/FunctionBreakpoint.h
index b15ff1931a6b22..93f0b93b35291d 100644
--- a/lldb/tools/lldb-dap/FunctionBreakpoint.h
+++ b/lldb/tools/lldb-dap/FunctionBreakpoint.h
@@ -10,14 +10,14 @@
#define LLDB_TOOLS_LLDB_DAP_FUNCTIONBREAKPOINT_H
#include "Breakpoint.h"
+#include "DAPForward.h"
namespace lldb_dap {
struct FunctionBreakpoint : public Breakpoint {
std::string functionName;
- FunctionBreakpoint() = default;
- FunctionBreakpoint(const llvm::json::Object &obj);
+ FunctionBreakpoint(DAP &dap, const llvm::json::Object &obj);
// Set this breakpoint in LLDB as a new breakpoint
void SetBreakpoint();
diff --git a/lldb/tools/lldb-dap/InstructionBreakpoint.cpp b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp
index e3a8460bb7b301..8c5c32c9270b41 100644
--- a/lldb/tools/lldb-dap/InstructionBreakpoint.cpp
+++ b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp
@@ -8,22 +8,25 @@
//===----------------------------------------------------------------------===//
#include "InstructionBreakpoint.h"
+
#include "DAP.h"
#include "JSONUtils.h"
namespace lldb_dap {
// Instruction Breakpoint
-InstructionBreakpoint::InstructionBreakpoint(const llvm::json::Object &obj)
- : Breakpoint(obj), instructionAddressReference(LLDB_INVALID_ADDRESS), id(0),
+InstructionBreakpoint::InstructionBreakpoint(DAP &d,
+ const llvm::json::Object &obj)
+ : Breakpoint(d, obj), instructionAddressReference(LLDB_INVALID_ADDRESS),
offset(GetSigned(obj, "offset", 0)) {
GetString(obj, "instructionReference")
.getAsInteger(0, instructionAddressReference);
instructionAddressReference += offset;
}
-void InstructionBreakpoint::SetInstructionBreakpoint() {
- bp = g_dap.target.BreakpointCreateByAddress(instructionAddressReference);
- id = bp.GetID();
+void InstructionBreakpoint::SetBreakpoint() {
+ bp = dap.target.BreakpointCreateByAddress(instructionAddressReference);
+ Breakpoint::SetBreakpoint();
}
+
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/InstructionBreakpoint.h b/lldb/tools/lldb-dap/InstructionBreakpoint.h
index 53912af46ca148..cc251c96f5bdd8 100644
--- a/lldb/tools/lldb-dap/InstructionBreakpoint.h
+++ b/lldb/tools/lldb-dap/InstructionBreakpoint.h
@@ -10,7 +10,12 @@
#ifndef LLDB_TOOLS_LLDB_DAP_INSTRUCTIONBREAKPOINT_H
#define LLDB_TOOLS_LLDB_DAP_INSTRUCTIONBREAKPOINT_H
+#include <cstdint>
+
+#include "lldb/lldb-types.h"
+
#include "Breakpoint.h"
+#include "DAPForward.h"
namespace lldb_dap {
@@ -18,16 +23,15 @@ namespace lldb_dap {
struct InstructionBreakpoint : public Breakpoint {
lldb::addr_t instructionAddressReference;
- int32_t id;
int32_t offset;
- InstructionBreakpoint()
- : Breakpoint(), instructionAddressReference(LLDB_INVALID_ADDRESS), id(0),
+ InstructionBreakpoint(DAP &d)
+ : Breakpoint(d), instructionAddressReference(LLDB_INVALID_ADDRESS),
offset(0) {}
- InstructionBreakpoint(const llvm::json::Object &obj);
+ InstructionBreakpoint(DAP &d, const llvm::json::Object &obj);
// Set instruction breakpoint in LLDB as a new breakpoint
- void SetInstructionBreakpoint();
+ void SetBreakpoint();
};
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 97fe6b4f9f05db..70bebddc91119d 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -831,70 +831,6 @@ llvm::json::Value CreateExtendedStackFrameLabel(lldb::SBThread &thread) {
{"presentationHint", "label"}});
}
-// Response to `setInstructionBreakpoints` request.
-// "Breakpoint": {
-// "type": "object",
-// "description": "Response to `setInstructionBreakpoints` request.",
-// "properties": {
-// "id": {
-// "type": "number",
-// "description": "The identifier for the breakpoint. It is needed if
-// breakpoint events are used to update or remove breakpoints."
-// },
-// "verified": {
-// "type": "boolean",
-// "description": "If true, the breakpoint could be set (but not
-// necessarily at the desired location."
-// },
-// "message": {
-// "type": "string",
-// "description": "A message about the state of the breakpoint.
-// This is shown to the user and can be used to explain why a breakpoint
-// could not be verified."
-// },
-// "source": {
-// "type": "Source",
-// "description": "The source where the breakpoint is located."
-// },
-// "line": {
-// "type": "number",
-// "description": "The start line of the actual range covered by the
-// breakpoint."
-// },
-// "column": {
-// "type": "number",
-// "description": "The start column of the actual range covered by the
-// breakpoint."
-// },
-// "endLine": {
-// "type": "number",
-// "description": "The end line of the actual range covered by the
-// breakpoint."
-// },
-// "endColumn": {
-// "type": "number",
-// "description": "The end column of the actual range covered by the
-// breakpoint. If no end line is given, then the end column is assumed to
-// be in the start line."
-// },
-// "instructionReference": {
-// "type": "string",
-// "description": "A memory reference to where the breakpoint is set."
-// },
-// "offset": {
-// "type": "number",
-// "description": "The offset from the instruction reference.
-// This can be negative."
-// },
-// },
-// "required": [ "id", "verified", "line"]
-// }
-llvm::json::Value CreateInstructionBreakpoint(BreakpointBase *ibp) {
- llvm::json::Object object;
- ibp->CreateJsonObject(object);
- return llvm::json::Value(std::move(object));
-}
-
// "Thread": {
// "type": "object",
// "description": "A Thread",
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 54fc4323475723..43056f3dc14566 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -380,17 +380,6 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame);
/// definition outlined by Microsoft.
llvm::json::Value CreateExtendedStackFrameLabel(lldb::SBThread &thread);
-/// Create a "instruction" object for a LLDB disassemble object as described in
-/// the Visual Studio Code debug adaptor definition.
-///
-/// \param[in] bp
-/// The LLDB instruction object used to populate the disassembly
-/// instruction.
-/// \return
-/// A "Scope" JSON object with that follows the formal JSON
-/// definition outlined by Microsoft.
-llvm::json::Value CreateInstructionBreakpoint(BreakpointBase *ibp);
-
/// Create a "Thread" object for a LLDB thread object.
///
/// This function will fill in the following keys in the returned
diff --git a/lldb/tools/lldb-dap/SourceBreakpoint.cpp b/lldb/tools/lldb-dap/SourceBreakpoint.cpp
index d1a3a5bedb0ae2..7415a0914dad43 100644
--- a/lldb/tools/lldb-dap/SourceBreakpoint.cpp
+++ b/lldb/tools/lldb-dap/SourceBreakpoint.cpp
@@ -12,15 +12,16 @@
namespace lldb_dap {
-SourceBreakpoint::SourceBreakpoint(const llvm::json::Object &obj)
- : Breakpoint(obj), logMessage(std::string(GetString(obj, "logMessage"))),
+SourceBreakpoint::SourceBreakpoint(DAP &dap, const llvm::json::Object &obj)
+ : Breakpoint(dap, obj),
+ logMessage(std::string(GetString(obj, "logMessage"))),
line(GetUnsigned(obj, "line", 0)), column(GetUnsigned(obj, "column", 0)) {
}
void SourceBreakpoint::SetBreakpoint(const llvm::StringRef source_path) {
lldb::SBFileSpecList module_list;
- bp = g_dap.target.BreakpointCreateByLocation(source_path.str().c_str(), line,
- column, 0, module_list);
+ bp = dap.target.BreakpointCreateByLocation(source_path.str().c_str(), line,
+ column, 0, module_list);
if (!logMessage.empty())
SetLogMessage();
Breakpoint::SetBreakpoint();
@@ -279,7 +280,7 @@ void SourceBreakpoint::SetLogMessage() {
void SourceBreakpoint::NotifyLogMessageError(llvm::StringRef error) {
std::string message = "Log message has error: ";
message += error;
- g_dap.SendOutput(OutputType::Console, message);
+ dap.SendOutput(OutputType::Console, message);
}
/*static*/
@@ -304,14 +305,16 @@ bool SourceBreakpoint::BreakpointHitCallback(
frame.GetValueForVariablePath(expr, lldb::eDynamicDontRunTarget);
if (value.GetError().Fail())
value = frame.EvaluateExpression(expr);
- output += VariableDescription(value).display_value;
+ output +=
+ VariableDescription(value, bp->dap.enable_auto_variable_summaries)
+ .display_value;
} else {
output += messagePart.text;
}
}
if (!output.empty() && output.back() != '\n')
output.push_back('\n'); // Ensure log message has line break.
- g_dap.SendOutput(OutputType::Console, output.c_str());
+ bp->dap.SendOutput(OutputType::Console, output.c_str());
// Do not stop.
return false;
diff --git a/lldb/tools/lldb-dap/SourceBreakpoint.h b/lldb/tools/lldb-dap/SourceBreakpoint.h
index aa3fbe6d0f96d2..113c0efbddcc5c 100644
--- a/lldb/tools/lldb-dap/SourceBreakpoint.h
+++ b/lldb/tools/lldb-dap/SourceBreakpoint.h
@@ -31,8 +31,7 @@ struct SourceBreakpoint : public Breakpoint {
uint32_t line; ///< The source line of the breakpoint or logpoint
uint32_t column; ///< An optional source column of the breakpoint
- SourceBreakpoint() : Breakpoint(), line(0), column(0) {}
- SourceBreakpoint(const llvm::json::Object &obj);
+ SourceBreakpoint(DAP &d, const llvm::json::Object &obj);
// Set this breakpoint in LLDB as a new breakpoint
void SetBreakpoint(const llvm::StringRef source_path);
diff --git a/lldb/tools/lldb-dap/Watchpoint.cpp b/lldb/tools/lldb-dap/Watchpoint.cpp
index 21765...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Instead, when a breakpoint is constructed it will be passed a DAP reference that it should use for its lifetime. This is part of a larger refactor to remove the global `g_dap` variable in favor of managing instances.
JDevlieghere
reviewed
Nov 6, 2024
labath
reviewed
Nov 7, 2024
There was a problem hiding this comment.
This PR is much easier to review. I mainly focused on the stylistic parts. I'll leave it to the lldb-dap maintainers to review the functionality.
Copying polymorphic objects around is asking for trouble, and I don't think we should be doing that. You didn't come up with that, and it's unrelated to what you're trying to do, so I'm not asking you to change that. I'm just mentioning this in case you feel the urge to do something about.
Co-authored-by: Pavel Labath <[email protected]>
With these changes, and the fact that If I did follow up with another refactor on this, do you have any suggestions on how to change that? Should we separate the breakpoint representation from the SBBreakpoint instance? I do wonder if we should further refactor the types in lldb-dap to be a little more like clangd where we have a more distinct protocol representation with |
I don't see that as a problem, but if at some point we need some more flexibility, we should be able to use std::reference_wrapper to store the ref and then allow for copies of objects when it makes sense.
100%! |
walter-erquinigo
approved these changes
Nov 8, 2024
Ah, I see. The (unused, I guess) BreakpointBase copy constructor threw me off. In this case, I'm happy.
I don't feel I know the code well enough to answer this (at this point, I think you're more familiar with lldb-dap than I am), but I certainly see a lot of room for improvement here. One of the things that really struck me is how all of these breakpoint objects are marked as "structs" -- they have public fields and everything, but then they also have nontrivial internal logic and virtual methods and whatnot. If we want these to be structs, then they should be just data holders and the logic should be somewhere else (I guess that's something along the lines of what you're suggesting), or they should be classes and their data ought to be private. |
labath
approved these changes
Nov 8, 2024
|
I remember that when lldb-dap was being brought up, the breakpoint structs were very simple, but they have become non-trivial at this point, so I second the idea of turning them into proper classes with better data encapsulation. |
Co-authored-by: Pavel Labath <[email protected]>
Groverkss
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Nov 15, 2024
…lvm#115208) Refactoring breakpoints to not use the `g_dap` reference. Instead, when a breakpoint is constructed it will be passed a DAP reference that it should use for its lifetime. This is part of a larger refactor to remove the global `g_dap` variable to allow us to create multiple DAP instances. --------- Co-authored-by: Pavel Labath <[email protected]>
JDevlieghere
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 25, 2025
…lvm#115208) Refactoring breakpoints to not use the `g_dap` reference. Instead, when a breakpoint is constructed it will be passed a DAP reference that it should use for its lifetime. This is part of a larger refactor to remove the global `g_dap` variable to allow us to create multiple DAP instances. --------- Co-authored-by: Pavel Labath <[email protected]> (cherry picked from commit b99d411)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring breakpoints to not use the
g_dapreference.Instead, when a breakpoint is constructed it will be passed a DAP reference that it should use for its lifetime.
This is part of a larger refactor to remove the global
g_dapvariable to allow us to create multiple DAP instances.