[Lldb-commits] [PATCH] D13073: Add an expression parser for Go

[Jim Ingham via lldb-commits]

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