xiaobai added inline comments.
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:341
+ "\n"
+ " typedef unsigned int uint32_t;\n"
+ " typedef unsigned long long uint64_t ;\n"
----------------
labath wrote:
> aprantl wrote:
> > This seems awfully specific. Shouldn't this be target-dependent, and is
> > there no pre-existing table in LLDB that we could derive this from?
> This entire function is target specific. I would struggle to find a single
> line in this expression that could be reused on linux for instance (and let's
> not even talk about windows..).
> I still believe that the simplest approach here would be to allocate memory
> for this structure on the stack, side-stepping the whole "how to allocate
> memory in a target-independent manner" discussion completely.
As others have state, this is entirely target-specific. I'd really rather avoid
behavior specific to one platform in something a general as lldbBreakpoint.
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:12
+#include "Plugins/ExpressionParser/Clang/ClangExpressionVariable.h"
+#include "Plugins/ExpressionParser/Clang/ClangUserExpression.h"
+#include "lldb/Expression/ExpressionVariable.h"
----------------
Please don't introduce dependencies on Plugins in non-plugin libraries. I'm
working on removing them.
Is BreakpointInjectedSite inherently tied to clang? If so, why not move it to
the ClangExpressionParser plugin? If not, there should be some kind of
generalization to ExpressionParser and PersistentExpressions.
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:246
+ ClangUserExpression *clang_expr =
+ llvm::dyn_cast<ClangUserExpression>(m_condition_expression_sp.get());
+
----------------
labath wrote:
> This also looks like an improper core code -> plugin dependency.
Indeed it is. Let's find a way to avoid that.
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:264
+ for (uint32_t i = 0; i < num_elements; ++i) {
+ const clang::NamedDecl *decl = nullptr;
+ llvm::Value *value = nullptr;
----------------
Could you use CompilerDecl here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66249/new/
https://reviews.llvm.org/D66249
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
