-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assertion failure when using group-by and amount #2330
Comments
I would like to fix this bug, I can see the problem is that in I don't understand how it got to this state. This is where it starts to recurse forever: https://github.com/ledger/ledger/blob/master/src/scope.h#L188
Here is the debug output before that happens:
This is pretty deep in the weeds, any hints @jwiegley 🤣 |
Could you share the call stack until the |
@igbanam I can't parse what you're asking, what It looks like @xkikeg spotted the problem using ASan in #2343, but didn't have a solution. Is the issue just that we need to restore the context after the calc(), because the diff --git a/src/post.cc b/src/post.cc
index 72930b46..e2063a36 100644
--- a/src/post.cc
+++ b/src/post.cc
@@ -642,11 +642,12 @@ void post_t::add_to_value(value_t& value, const optional<expr_t&>& expr) const
add_or_set_value(value, xdata_->compound_value);
}
else if (expr) {
- bind_scope_t bound_scope(*expr->get_context(),
- const_cast<post_t&>(*this));
+ scope_t *ctx = expr->get_context();
+ bind_scope_t bound_scope(*ctx, const_cast<post_t&>(*this));
#if 1
value_t temp(expr->calc(bound_scope));
add_or_set_value(value, temp);
+ expr->set_context(ctx);
#else
if (! xdata_) xdata_ = xdata_t();
xdata_->value = expr->calc(bound_scope); That does seem to fix it, and all the tests parse... what do you think @jwiegley @xkikeg @tbm? I'll send you a pull request for that. |
Part of the expr_t::compile() process is to store the current scope, but In post_t::add_to_value that scope is temporary and on the stack. Restore the original context after that process is complete.
Oh I'm glad you two could figure it out. I was asking for more information. 2343 seems to have provided that |
Thanks! That looks like a reasonable fix, LGTM If we can invent a way to have a temporal evaluation, that'll be even cleaner, but I don't know if it's easy with the current |
Part of the expr_t::compile() process is to store the current scope, but In post_t::add_to_value that scope is temporary and on the stack. Restore the original context after that process is complete.
Test ledger File:
The text was updated successfully, but these errors were encountered: