Fl_Text_Buffer constructor bug

[Mark Olesen]

Fl_Text_Buffer::Fl_Text_Buffer(int requestedSize, int preferredGapSize)

{

  mLength = 0;

  mPreferredGapSize = preferredGapSize;

  mBuf = (char *) malloc(requestedSize + mPreferredGapSize);

  mGapStart = 0;

  mGapEnd = mPreferredGapSize;

mGapEnd does not take into account requestedSize. Should be the same as allocated size (requestedSize + mPreferredGapSize)

This is also the same problem in Nedit textBuf.C BufCreatePreallocated

[Albrecht Schlosser]

On 05.12.2016 14:58 Mark Olesen wrote:
> Fl_Text_Buffer::Fl_Text_Buffer(int requestedSize, int preferredGapSize)
> {
> mLength = 0;
> mPreferredGapSize = preferredGapSize;
> mBuf = (char *) malloc(requestedSize + mPreferredGapSize);
> mGapStart = 0;
> mGapEnd = mPreferredGapSize;
>
> mGapEnd does not take into account requestedSize. Should be the same as
> allocated size (requestedSize + mPreferredGapSize)

Good catch! Thanks for the report.

I quickly checked the header file and found that the default value of
requestedSize is 0, so the default allocation would be okay.

But, what is the effect if requestedSize > 0 ? I could check the code,
but since you likely did this already you may be able to answer the
question. I can imagine two scenarios:

(1) The "requestedSize" range is never used

(2) When the "gap" is filled this is somehow "auto-adjusted" because the
code checks the existing allocation and does "The Right Thing".

The first case would really be bad, whereas the second case wouldn't do
much harm.

> This is also the same problem in Nedit textBuf.C BufCreatePreallocated

That's probably the reason why it is this way in FLTK.

[Mark Olesen]

It appears that providing a requested size will just allocate more memory than will be utilized.  That is, the gap buffer will only use preferredGapSize bytes of memory. The rest will be wasted until reallocate_with_gap is called, which creates an entirely new buffer and deletes the old.

As a side note, Fl_Text_Display::buffer( Fl_Text_Buffer *buf ) does not appear to delete mBuffer if it is not null.

  if ( mBuffer != 0 ) {

    // we must provide a copy of the buffer that we are deleting!

    char *deletedText = mBuffer->text();

    buffer_modified_cb( 0, 0, mBuffer->length(), 0, deletedText, this );

    free(deletedText);

    mNBufferLines = 0;

    mBuffer->remove_modify_callback( buffer_modified_cb, this );

    mBuffer->remove_predelete_callback( buffer_predelete_cb, this );

   /* ---> here mBuffer should be deleted: delete mBuffer <---- */

  }

  /* Add the buffer to the display, and attach a callback to the buffer for

   receiving modification information when the buffer contents change */

  mBuffer = buf;

anyway....

[Mark Olesen]

It looks like Fl_Text_Display::buffer( Fl_Text_Buffer *buf ) does not free mBuffer by design: "A single Text Buffer can be displayed by multiple Text Displays." The destructor does not release it either. What a conundrum. So, please disregard my last note about this.

[Albrecht Schlosser]

On 05.12.2016 17:11 Mark Olesen wrote:
> It appears that providing a requested size will just allocate more
> memory than will be utilized. That is, the gap buffer will only use
> preferredGapSize bytes of memory. The rest will be wasted until
> reallocate_with_gap is called, which creates an entirely new buffer and
> deletes the old.

Okay, I see (checked). Thanks again for the report, I'll commit a fix in
FLTK 1.4 (branch-1.4).

> As a side note, Fl_Text_Display::buffer( Fl_Text_Buffer *buf ) does not
> appear to delete mBuffer if it is not null.
>
> if ( mBuffer != 0 ) {
> // we must provide a copy of the buffer that we are deleting!
> char *deletedText = mBuffer->text();
> buffer_modified_cb( 0, 0, mBuffer->length(), 0, deletedText, this );
> free(deletedText);
> mNBufferLines = 0;
> mBuffer->remove_modify_callback( buffer_modified_cb, this );
> mBuffer->remove_predelete_callback( buffer_predelete_cb, this );
> /* ---> here mBuffer should be deleted: delete mBuffer <---- */
> }
>
> /* Add the buffer to the display, and attach a callback to the buffer for
> receiving modification information when the buffer contents change */
> mBuffer = buf;

Well, it's hard to say if this is a bug or intentional. I believe it's
intentional though:

I checked the docs, and the behavior WRT the old (replaced) buffer is
not documented. However, the following *is* documented: "Multiple text
widgets can be associated with the same text buffer."

I take it that the Fl_Text_Display widget must not destroy the
associated Fl_Text_Buffer because the text buffer can still be
associated with another Fl_Text_Display or Fl_Text_Editor widget. Hence
it is the caller's responsibility to dispose of the text buffer once it
is no longer used [1]. I think this should be documented, but the code
is correct.

[1] It could also be used for something else, for instance to store (and
keep) the text in the buffer. That buffer could also be associated with
the same or another display or editor widget later.

Comments, anybody?

> anyway...

... thanks for the report.

[Albrecht Schlosser]

No (to "disregard..."). Thanks for the report and also for the
correction (see my other posting as well). The fact that it was not
clear shows a lack of documentation. The docs should be fixed.

[Albrecht Schlosser]

On 05.12.2016 17:45 Albrecht Schlosser wrote:
> On 05.12.2016 17:11 Mark Olesen wrote:
>> It appears that providing a requested size will just allocate more
>> memory than will be utilized. That is, the gap buffer will only use
>> preferredGapSize bytes of memory. The rest will be wasted until
>> reallocate_with_gap is called, which creates an entirely new buffer and
>> deletes the old.
>
> Okay, I see (checked). Thanks again for the report, I'll commit a fix in
> FLTK 1.4 (branch-1.4).

Done, see svn r12134.

[Albrecht Schlosser]

Done, see svn r12135.