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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
