Patch for bug #103174

[Nigel Titley]

On Thu, 2003-09-04 at 18:04, Derek Atkins wrote:
> Nigel Titley <nigel at titley.com> writes:
> 
> > > Am I missing something in your patch?
> > 
> > Yes you are. The problem is that the SX formula_cell stuff calls (via an
> > intermediate step) 
> > 
> > gnc_basic_cell_set_value_internal (BasicCell *cell, const char *value)
> > 
> > with *value pointing at cell->value. The overall effect of this as far
> > as gnc_basic_cell_set_value_internal is concerned is to trash the value
> > stored in cell->value *before* re-writing back to the same place (the
> > g_free() writes memory management stuff in the first few bytes of the
> > value). This is what trashes the debit/credit value in the SX register
> > entry. Now there are at least three ways to fix this:
> 
> Aha..  yes, that does make it more clear...

Good... I was beginning to doubt myself for a moment there

> > 1. Modify the formula cell code to save cell->value in a temporary value
> > 
> > 2. Add a test in gnc_basic_cell_set_value_internal() to not do the
> > assignment in the case where the source and destination are the same
> > 
> > 3. Store the value in a temporary variable to avoid the overwrite.
> 
> Yep, I agree with these options...
> 
> > I decided on 2 or 3 because it makes gnc_basic_cell_set_value_internal()
> > safer. Whether you prefer a solution based on strategy 2 or 3 depends on
> > your mindset. I happen to prefer 3, hence the patch.
> 
> I think #2 would be better because it's one fewer malloc/free.  Basically:
> 
>    if (cell->value == value)
>       return;

Well, not quite, because there are some further operations to do which
need doing even if we don't set the cell value, but yes, agreed in
principle. 

> It's a faster operation than unconditionally malloc/freeing.  Although
> the flip-side of this argument is that the NORMAL case is the
> free/malloc, so it's adding a useless test in the normal case.  So I
> can go either way.

Yes, there isn't a lot in it. I considered that the normal case would
result in a useless test and that a simple copy to a stack variable
would probably be much the same performance wise. However I agree that
there would be a marginal performance improvement when doing a formula
cell edit. However this is hardly a compute intensive bit of the code.
And we've probably spent more time arguing over it than will ever be
saved one way or the other :-)

> > Sorry it has taken me so long to reply to this. I wasn't subscribed to
> > gnucash-patches. I've been sitting here getting slightly upset that I've
> > had no response..... my fault. I eventually decided to check the archive
> > and saw your question. 
> 
> Strange -- I thought I sent it to you directly as well as to the list.

Hmm, don't seem to have had it. Still, no matter.

> > I've now subscribed.... if someone could either check in the patch or
> > let me know what you would like me to do. This patch fixes 4 extant bugs
> > in the SX part of bugzilla.
> 
> May I ask your reasoning for choosing #3 over #2?  If we continue down
> the road of #2 we should add a comment that explains why.  (If we
> choose #2 I think it's quite clear what you're doing).

Well, I think the patch included a comment. As to my reasoning, see
above. I really don't think there is much in it, and I found #3 more
intuitive (I find control flow changes more prone to error than
consecutive code) but that is really just my coding style, and I think
you probably need a comment in either case. Having said that I really
don't care which route we go. The important thing is to have found the
bug. If you want to check in your own fix then do so. Just let me know
so I can update bugzilla appropriately.

> Thanks!

No problem. 

All the best

Nigel
-- 
Nigel Titley <nigel at titley.com>