[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: include/lldb/API/SBValue.h:310-312
+  lldb::SBValue EvaluateExpression(const char *expr) const;
+  lldb::SBValue EvaluateExpression(const char *expr,
+                                   const SBExpressionOptions &options) const;
----------------
Can you use `StringRef` instead of `const char *` here?


================
Comment at: include/lldb/Expression/UserExpression.h:296
+           lldb::ModuleSP *jit_module_sp_ptr = nullptr,
+           const lldb::ValueObjectSP &ctx_obj = lldb::ValueObjectSP());
 
----------------
A reference to a `shared_ptr` seems odd.  Why don't we just say `const ValueObject* ctx_obj = nullptr`?


================
Comment at: include/lldb/Symbol/ClangASTContext.h:1057
+                    const EvaluateExpressionOptions &options,
+                    const lldb::ValueObjectSP &ctx_obj) override;
 
----------------
Same thing here.


================
Comment at: source/API/SBValue.cpp:1325-1327
+    if (log)
+      log->Printf(
+          "SBValue::EvaluateExpression called with an empty expression");
----------------
Instead of the lines like `if (log) log->Printf(...)` you can instead write:

```
LLDB_LOG(log, "SBValue::EvaluateExpression called with an empty expression");
```


================
Comment at: source/API/SBValue.cpp:1367-1371
+  if (log)
+    log->Printf("SBValue(%p)::EvaluateExpression (expr=\"%s\") => SBValue(%p) "
+                "(execution result=%d)",
+                static_cast<void *>(value_sp.get()), expr,
+                static_cast<void *>(res_val_sp.get()), expr_res);
----------------
If you decide to make that change, note that the macros call `Format`, not `Printf`, so in this case you would have to change your format string to something like:

```
LLDB_LOG(log, "SBValue({0}::EvaluateExpression (expr=\"{1}\") => SBValue({2}) (execution result = {3})",
    value_sp.get(), expr, res_val_sp.get(), expr_res);
```

BTW, I would discourage logging pointer values, as it makes log output non-deterministic and doesn't often help when reading log files.  That said, it wouldn't be the first time we logged pointer values.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55318/new/

https://reviews.llvm.org/D55318