bug-ncurses
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [CDK] A few problems, remarks and suggestions, mostly concerning the


From: Stéphane Goujet
Subject: Re: [CDK] A few problems, remarks and suggestions, mostly concerning the Scroll widget
Date: Mon, 31 Dec 2018 17:04:15 +0100 (CET)
User-agent: Alpine 2.21.1 (LNX 216 2017-09-19)

Hello,

First, thank you for having taken the time to read and answer my remarks.


On Sat, 29 Dec 2018, Thomas Dickey wrote:

On Wed, Dec 26, 2018 at 03:34:12PM +0100, Stéphane Goujet wrote:
Hello,


  Here is a list of a few problems I found while using cdk-5.0-20180306:

  For the first 2 cases (bug problems), I haven't written test cases, but if
you request it, I shall do it.



1. In "scroll.c", function allocListArrays(), around line 714:

You free "item" but not the content of the list it points to.

Instead of:

      freeChecked (scrollp->item);

shouldn't it be something like:

      CDKfreeChtypes (scrollp->item);

as it is done in _destroyCDKScroll() ?

offhand - no: the data has just been copied to a new array.
What's being freed is the obsolete array.

Oh, you are right, I was mistaken.

But then I still think there is a problem somewhere up in a caller. I'll explain the origin of my problem.

Here is what I had to create:


----------------------------------
void replaceCDKScrollItems (CDKSCROLL *scrollp, CDK_CSTRING2 list, int listSize, boolean numbers) {
        CDKfreeChtypes(scrollp->item);
        scrollp->item = NULL;

        setCDKScrollItems(scrollp, list, listSize, numbers);
}
----------------------------------


That's because setCDKScrollItems() calls createCDKScrollItemList(), which does:

----------------------------------
static int createCDKScrollItemList (CDKSCROLL *scrollp,
                                    boolean numbers,
                                    CDK_CSTRING2 list,
                                    int listSize)
{
   int status = 0;

   if (listSize > 0)
   {
      /* *INDENT-EQLS* */
      size_t have               = 0;
      char *temp                = 0;

      if (allocListArrays (scrollp, 0, listSize))
      {
----------------------------------


As we can see aboce and below, it calls allocListArrays() with oldSize=0. So the existing items are neither free'd nor copied in the new list (since we 'set' a new list, this last point is normal), their reference is lost.


----------------------------------
static boolean allocListArrays (CDKSCROLL *scrollp,
                                int oldSize,
                                int newSize)
{
   /* *INDENT-EQLS* */
   boolean result;
   int nchunk           = ((newSize + 1) | 31) + 1;
   chtype **newList     = typeCallocN (chtype *, nchunk);
   int *newLen          = typeCallocN (int, nchunk);
   int *newPos          = typeCallocN (int, nchunk);

   if (newList != 0 &&
       newLen != 0 &&
       newPos != 0)
   {
      int n;

      for (n = 0; n < oldSize; ++n)
      {
         newList[n] = scrollp->item[n];
         newLen[n] = scrollp->itemLen[n];
         newPos[n] = scrollp->itemPos[n];
      }

      freeChecked (scrollp->item);
      freeChecked (scrollp->itemPos);
      freeChecked (scrollp->itemLen);
----------------------------------


So, it means that setCDKScrollItems() can only be used cleanly on an empty CDKSCROLL. If a CDKSCROLL has already, in a way or another, been filled with items, those items will be left dangling.

And I don't think there is a correct way in the API to clean all items away before calling setCDKScrollItems() to set a new list of items. Is there? (I mean, besides repetively calling deleteCDKScrollItem() for each item...)


Here's a test program (running it through Valgrind will show the leak):

-----------------------------------
#include <cdk.h>
#include <unistd.h>    // sleep()

int main(void) {
        CDKSCREEN    *cdkscreen;
        WINDOW       *cursesWin;
        CDKSCROLL    *scrlist;
        int choice;
        char *veglist[] = {"Artichoke","Bean","Cabbage","Onion"};
        char *furnlist[] = {"Bed","Chair","Table"};

        cursesWin = initscr();
        cdkscreen = initCDKScreen(cursesWin);

        initCDKColor();
        scrlist = newCDKScroll(
                cdkscreen,
                CENTER,
                CENTER,
                RIGHT,
                8,
                30,
                "Test",
                veglist,
                4,
                FALSE,
                A_REVERSE,
                TRUE,
                FALSE
        );

        refreshCDKScreen(cdkscreen);
        sleep(2);
        eraseCDKScroll(scrlist);

        /* uncomment the following 2 lines to solve the problem: */
//      CDKfreeChtypes(scrlist->item);
//      scrlist->item = NULL;

        setCDKScrollItems(scrlist, furnlist, 3, FALSE);
        refreshCDKScreen(cdkscreen);

        while(scrlist->exitType != vNORMAL) {
                activateCDKScroll(scrlist, 0);
        }

        destroyCDKScroll(scrlist);

        destroyCDKScreen(cdkscreen);

        endCDK();

        return 0;

}
-----------------------------------


So, as I understand it, there are several roads that can be taken:

1. Leave setCDKScrollItems() as it is (but document that it can only be called once, and only on a blank CDKSCROLL), and add a replaceCDKScrollItems() function (as I did, but inside the API).

2. Fix setCDKScrollItems() to handle cases when the CDKSCROLL already contains items. Then it behaves like a replaceCDKScrollItems() function.

3. Add a cleanCDKScrollItems() function, which deletes items. setCDKScrollItems() remains as it is (but its limitations are documented). Implementing a replaceCDKScrollItems() equivalent is left to the user.

What's your opinion?



4. Documentation and code (man cdk_scroll; "scroll.c", line 867)

getCDKScrollItems(): the man page doesn't mention that NULL is allowed for
the list parameter, and that it is a way to get the number of items in the
scroll list.

I added the null-pointer check in 2005, apparently after seeing something
break.  I suppose documenting it would help :-)

BTW, I think it is (is it?) the only way to get the number of items, which
is weird. A dedicated function would be much more natural.

Was I right, that there is no official function to get the number of items in the scroll list? (besides using this one in this 'unexpected' way)



getchCDKObject() replaces it, but it has an extra argument: boolean
*functionKey, and this pointer is made mandatory.

What if I don't care about this functionKey indicator? Don't you think I should be allowed to pass NULL to mean "don't send me back this indicator" ?

This would just requested to insert a test into the function
getchCDKObject() ("binding.c", line 228) before setting the indicator:

   if(functionKey) { //  <--- add this test
       *functionKey = (key >= KEY_MIN && key <= KEY_MAX);
   }

ok :-)

Cool, now I just have to convince the C and POSIX committees to do the same for functions like fgets() or read() ;-)



Faithfully yours,
  Stéphane.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]