[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a comment.

I really like the solution, but I think by fixing the `CanInterpret` you also made the test case no longer reach the actual changed interpreting logic?

So, `CanInterpret` says "we can't interpret this" (which is correct), but then the interpreter won't run it and your change to `ResolveConstantValue` isn't actually tested. There is no other test that touches that logic from what I can see. You could try throwing in some other expression at this that tests that new code? Maybe some kind of pointer arithmetic on a variable defined in your expression itself (so it can be constant evaluated). We could also split out the interpreting change and then this is good to go.

Also I think a second set of eyes on this would be nice as I rarely touch the IRInterpreter, but not sure who the best person for that is. I'll add the LLDB group and let's see if anyone has a second opinion on this, but in general this LGTM module the test situation.



================
Comment at: lldb/source/Expression/IRInterpreter.cpp:286
           SmallVector<Value *, 8> indices(op_cursor, op_end);
+          SmallVector<Value *, 8> const_indices;
+
----------------
Maybe `resolved_indices`? `const_` always sounds a bit like it's meaning 'const' qualified version of indices or so.


================
Comment at: lldb/source/Expression/IRInterpreter.cpp:288
+
+          for (Value *idx : indices) {
+            Constant *constant_idx = dyn_cast<Constant>(idx);
----------------
I think this deserves a comment that `getIndexedOffsetInType` can only handle ConstantInt indices (and that's why we're resolving them here).


================
Comment at: lldb/source/Expression/IRInterpreter.cpp:490
+                                         constant_expr->op_end());
+        for (Value *op : operands) {
+          Constant *constant_op = dyn_cast<Constant>(op);
----------------
`for (Value *op : constant_expr->ops())` ?


================
Comment at: lldb/source/Expression/IRInterpreter.cpp:494
+            return false;
+          if (!CanResolveConstant(constant_op)) {
+            return false;
----------------
nit no `{}` for single line ifs


================
Comment at: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py:71
+        value = self.target().EvaluateExpression(expr)
+        self.assertTrue(value.GetError().Success())
----------------
`self.assertSuccess` (that will print the error on failure to the test log)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113498