jingham added a comment.
The generic parts of this change look fine to me, with a few inlined style
comments.
I didn't read the Go specific parts of this in detail, I assume you're going to
get those right. I mentioned a couple of style things inline, though I didn't
mark everywhere that they occurred. Particularly, the construct:
Foo::Foo () :
m_ivar1
, m_ivar2
etc. looks very odd, and is not the way we do it anywhere else in lldb. Please
don't write it that way.
Also, we try to always call shared pointers foo_sp and unique pointers foo_up.
The life cycle of these guys is enough different from pointers that it is good
to be able to see what they are in code without having to trace them back to
the definition.
With those style changes this is okay by me.
================
Comment at: source/Expression/LLVMUserExpression.cpp:43-60
@@ +42,20 @@
+
+LLVMUserExpression::LLVMUserExpression(ExecutionContextScope &exe_scope, const
char *expr, const char *expr_prefix,
+ lldb::LanguageType language, ResultType
desired_type)
+ : UserExpression(exe_scope, expr, expr_prefix, language, desired_type)
+ , m_stack_frame_bottom(LLDB_INVALID_ADDRESS)
+ , m_stack_frame_top(LLDB_INVALID_ADDRESS)
+ , m_transformed_text()
+ , m_execution_unit_sp()
+ , m_materializer_ap()
+ , m_jit_module_wp()
+ , m_enforce_valid_object(true)
+ , m_in_cplusplus_method(false)
+ , m_in_objectivec_method(false)
+ , m_in_static_method(false)
+ , m_needs_object_ptr(false)
+ , m_const_object(false)
+ , m_target(NULL)
+ , m_can_interpret(false)
+ , m_materialized_address(LLDB_INVALID_ADDRESS)
+{
----------------
Don't write initializers with the commas in front like this. We don't do it
this way anywhere else in lldb, and it looks really weird...
================
Comment at: source/Expression/UserExpression.cpp:48-54
@@ -47,27 +47,9 @@
-UserExpression::UserExpression (ExecutionContextScope &exe_scope,
- const char *expr,
- const char *expr_prefix,
- lldb::LanguageType language,
- ResultType desired_type) :
- Expression (exe_scope),
- m_stack_frame_bottom (LLDB_INVALID_ADDRESS),
- m_stack_frame_top (LLDB_INVALID_ADDRESS),
- m_expr_text (expr),
- m_expr_prefix (expr_prefix ? expr_prefix : ""),
- m_language (language),
- m_transformed_text (),
- m_desired_type (desired_type),
- m_execution_unit_sp(),
- m_materializer_ap(),
- m_jit_module_wp(),
- m_enforce_valid_object (true),
- m_in_cplusplus_method (false),
- m_in_objectivec_method (false),
- m_in_static_method(false),
- m_needs_object_ptr (false),
- m_const_object (false),
- m_target (NULL),
- m_can_interpret (false),
- m_materialized_address (LLDB_INVALID_ADDRESS)
+UserExpression::UserExpression(ExecutionContextScope &exe_scope, const char
*expr, const char *expr_prefix,
+ lldb::LanguageType language, ResultType
desired_type)
+ : Expression(exe_scope)
+ , m_expr_text(expr)
+ , m_expr_prefix(expr_prefix ? expr_prefix : "")
+ , m_language(language)
+ , m_desired_type(desired_type)
{
----------------
Ditto, we don't write initializers this way. Move the commas to the end of the
line.
================
Comment at: source/Plugins/ExpressionParser/Go/GoAST.h:208-209
@@ +207,4 @@
+ : GoASTExpr(eArrayType)
+ , m_len(len)
+ , m_elt(elt)
+ {
----------------
Same comment about formatting...
================
Comment at: source/Plugins/ExpressionParser/Go/GoAST.h:250-251
@@ +249,4 @@
+ friend class GoASTNode;
+ std::unique_ptr<GoASTExpr> m_len;
+ std::unique_ptr<GoASTExpr> m_elt;
+
----------------
We usually postpend with _sp or _up variables that are shared or unique
pointers. They have sufficiently interesting lifecycle that it is helpful to
know at a glance what kind of thing they are.
================
Comment at: source/Plugins/ExpressionParser/Go/GoAST.h:262-263
@@ +261,4 @@
+ : GoASTStmt(eAssignStmt)
+ , m_define(define)
+ {
+ }
----------------
Here too...
================
Comment at: source/Plugins/ExpressionParser/Go/GoAST.h:324-325
@@ +323,4 @@
+ friend class GoASTNode;
+ std::vector<std::unique_ptr<GoASTExpr>> m_lhs;
+ std::vector<std::unique_ptr<GoASTExpr>> m_rhs;
+ bool m_define;
----------------
Maybe m_lhs_up, etc.
Repository:
rL LLVM
http://reviews.llvm.org/D13073
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
