Re: [aspell-devel] Aspell core dump

[Gary Setter]

----- Original Message ----- 
From: "Kevin Atkinson" <address@hidden>
To: "Gary Setter" <address@hidden>
Cc: <address@hidden>
Sent: Wednesday, December 15, 2004 11:42 PM
Subject: Re: [aspell-devel] Aspell core dump


> On Wed, 15 Dec 2004, Kevin Atkinson wrote:
>
> > On Wed, 15 Dec 2004, Gary Setter wrote:
> >
> > > <snip>
> > > > Hi,
> > > > I tried to look into this. The win32 port does not dump
core, but
> > > > it does reprompt for when webserver is replaced by
web-server or
> > > > web server. I have had it stuck in an infinite loop, but
I'm
> > > > having difficulty recreating the problem.
> >
> > So does Aspell hang because it is in an infinite loop or is
it just keep on
> > repromting?
>
> And when spell checking this message I believe I encountered
the same bug.
> Actually it was with repl_next but repl_next and
soundslike_next are
> nearly identical and they suffered from the same problem.  To
reproduce it
> try spellchecking the word "simpiler" with the attached
personal
> replacement file.
>
> Attached is a fix.

Thanks for the patch. My loop was the same as yours.

> > > > At the center of the infinite loop problem is this
function in
> > > > writeable.
> > > >
> > > > static void soundslike_next(WordEntry * w)
> > > > {
> > > >   const Str * i   = (const Str *)(w->intr[0]);
> > > >   const Str * end = (const Str *)(w->intr[1]);
> > > >   set_word(*w, *i);
> > > >   ++i;
> > > >   if (i == end) w->adv_ = 0;
> > > > }
> > > > Nothing is changing w->intr[0] or w-inter[1] so w->adv is
never
> > > > set to zero and the loop never terminates.
>
> Yep that is the cause.
>
> > > > Also the WordEntry::intr array of void pointers is a bad
idea in
> > > > my opinion. Is there any support for eliminating them?
>
> Well if you can figure out a better way without losing
efficiency.
>
I suspect that there is a better way that is nearly as efficient.
You know aspell best, but doesn't it seem to you that it is the
process of breakup up words and making letter by letter
substitutions is where the cpu cycles are going?

I'll express my thoughts at the end of this message.

> > > I'm still studying writable.cpp. I'm finding what are in my
> > > opinion bad ideas. This is the first:
> > > static inline StrVector * get_vector(Str s)
> > > {
> > >   return (StrVector *)(s - sizeof(StrVector) - 2);
> > > }
> > > It seems a Vector<Str> object is allocated, but only the
address
> > > of first string is saved. Then, when we want to use the
Vector,
> > > we just subtract and cast.
> > > I think the
> > >  WritableReplDict::add_repl()
> > > and
> > >  WritableDict::add()
> > > functions are along the same lines.
> > > Does anyone else have problems with these functions as they
are?
> > >
> > > I would like to straighten this out as part of fixing the
> > > infinite loop problem.
> >
> > There is nothing wrong with them.  I do it to save space.
Yes it
> > contains some ugly stuff but it is completely contained
within the single
> > source file so I don't have to worry about it anywhere else.
I will not
> > unless it causes a problem.
>
> I will however accept patches to improve the documentation.
Actually I
> don't think the reason was to save space but to simplify some
things.  In
> any case I will not change it unless you have a solution which
is
> simpler and doesn't use any additional space or it is causing a
problem.
>
My thoughts at the end of the message.

> > > Also, I had a problem saving replacement pairs that
included
> > > hyphenated words. I fixed it in
> > >   PosibErr<void> SpellerImpl::store_replacement().
> > > The fix was to retrieve the separator characters from
config and
> > > use them, (not just space) to break up the replacement
string
> > > into its parts. I wasn't going to submit a patch until I
fixed
> > > the infinite loop bug, but I would be interested in what
the list
> > > things before I submit a patch.
> >
> > I believe that is the correct thing to do but I will have to
see the patch
> > to be sure.
> >
As soon as I get the results that I expect, I'll submit a patch.

My thoughts:
If the sole purpose of making software free and open was to make
a solution available to all, then I would agree that there is
nothing wrong with how it is coded.

However, if the purpose is to improve confidence in the software,
and to open it up for improvement, and to make it available to be
adapted to new purposes, then I would disagree.

I would say that the code is difficult to understand. It does not
seem to me to be "solid code". To make a change, like adding a
field to whatever WritableBase::word_lookup holds would involve
exhaustive study of the whole module. Even after much study, you
would never be completely sure if your change broke someone
else's assumption. To just document it as it is would be
something of a cop out.