On Sun, 9 Oct 2016 21:40:55 +0000
Thomas Mannay <audiobarrier_AT_openmailbox.org> wrote:
Hey Thomas,
> > - the code within join() is not tab- but space-indented
> And here I thought I had set my editor to indent tabs...
this can happen, no problem.
> > - can getindex(curln-1) underflow? (if curln = 0)
> iirc currln can't be 0 since addresses start at 1 in ed. I've
> experienced no issues with joining a set of addresses beginning with
> the first line.
Alright, that sounds like a reasonable point. But what if curln is 1,
then the line index passed to getindex is "0". Is that a valid line
index?
> > - what's the purpose of the free(s)?
> The original code had two frees, which I assume was due to a static
> char pointer. This would cause a double free if you did two join
> commands in a row. The free at the end of the function is required
> because addchar returns a manually allocated char pointer.
Thanks for elaborating! I somehow forgot to notice the "static" for s.
> > - can you give an example in the patch-description where this
> > infinite loop occurs?
> I'm unsure where the patch description would be, but it occurs when
> you issue 1,2j when there are only two lines in the buffer. Doing
> that on the unpatched ed causes it to stop accepting commands until
> given an interrupt due to an infinite loop.
Alright, so the patch description is the "commit description" so to
say. It would be nice to have a minimal working example so people know
what the patch fixes and test it quickly and easily.
> > - Do we really need an "EXTENDED DESCRIPTION"
> Yes, it's one of the standard manpage subheadings and allowed me to
> properly document ed. It felt like The Right Thing since correct
> documentation is as important as correct code. It's actually how I
> found the bugs I fixed. :^)
Fair point!
> > - Please check the other manpages for the standard format of
> > the STANDARDS section
> Have amended this with the attached patch.
Alright, thanks!
> Should I redo those patches to account for changing commit messages
> to more active forms?
I would welcome this!
Cheers
Laslo
--
Laslo Hunhold <dev_AT_frign.de>
Received on Sun Oct 09 2016 - 23:48:15 CEST