Subject: builtins.c:AddTitleStyle() odd

From: "Paul D. Smith" <[email-masked]>
Date: Tue, 1 Dec 1998 22:22:31 -0800 (PST)

Can someone look at builtins.c:AddTitleStyle() and make sure it's doing
the right thing?

The loop seems odd to me; it loops through the action, parsing tokens
into "parm", but then that value is never used and instead the previous
token is passed to ReadTitleButton().  Is this right?  Obviously parm is
a memory leak, which I've fixed, but I wonder about the larger loop.

If that's the way it should work, it seems like a good enhancement to
GetNextToken() (really DoGetNextToken()) to allow the user to call it
with a NULL token pointer, if the caller doesn't want to bother
allocating memory for the returned token (which appears to be the case
here).  I can make that change if it seems like a good idea to others.

-- 
-------------------------------------------------------------------------------
 Paul D. Smith <[email-masked]>         Network Management Development
 "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
-------------------------------------------------------------------------------
   These are my opinions---Nortel Networks takes no responsibility for them.
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 2 Dec 1998 12:15:31 +0100

> a memory leak, which I've fixed, but I wonder about the larger loop.

Another bug. Replace the prev in ReadTitleButton with parm.

> here).  I can make that change if it seems like a good idea to others.

No need at the moment. There are already functions that do this
(PeekToken). I don't want to touch Parse.c right now. After the
release I'll try to think of some universal parsing functions.

Bye

Dominik
-- 
Dominik Vogt, Hewlett-Packard GmbH, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/14-4596, fax: 07031/14-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: [email-masked] (Paul D. Smith)
Date: 02 Dec 1998 09:46:50 -0500

%% Dominik Vogt <[email-masked]> writes:

  >> The loop seems odd to me; it loops through the action, parsing
  >> tokens into "parm", but then that value is never used and instead
  >> the previous token is passed to ReadTitleButton().  Is this right?
  >> Obviously parm is a memory leak, which I've fixed, but I wonder
  >> about the larger loop.

  dv> Another bug. Replace the prev in ReadTitleButton with parm.

OK... but in that case, what's prev for?  Now _IT_ doesn't ever seem to
be used for anything.

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 2 Dec 1998 16:47:21 +0100

> OK... but in that case, what's prev for?  Now _IT_ doesn't ever seem to

I don't know. Perhaps looking into the 47d or 46 code
will enlighten you. The function has obviously been
reworked (maybe by me, maybe not) but I can't find
it in the Changelog. My guess is that it is dead code.

Bye

Dominik

---

From: Jason L Tibbitts III <[email-masked]>
Date: 02 Dec 1998 09:58:58 -0600

>>>>> "DV" == Dominik Vogt <[email-masked]> writes:

DV> I don't know. Perhaps looking into the 47d or 46 code will enlighten
DV> you. The function has obviously been reworked (maybe by me, maybe not)
DV> but I can't find it in the Changelog.

I really recommend a cvs annotate or a look at
http://www.hpc.uh.edu/cgi-bin/cvsweb.cgi/fvwm/fvwm/builtins.c?annotate=1.30&cvsroot=FVWM
and a check of what changed in 1.11 and 1.13.  CVS can generally answer
these kinds of questions pretty well.

 - J<

---

From: [email-masked] (Paul D. Smith)
Date: 02 Dec 1998 11:42:42 -0500

%% Regarding Re: builtins.c:AddTitleStyle() odd;

  jlt> I really recommend a cvs annotate or a look at
  jlt> http://www.hpc.uh.edu/cgi-bin/cvsweb.cgi/fvwm/fvwm/builtins.c?annotate=1.30&cvsroot=FVWM
  jlt> and a check of what changed in 1.11 and 1.13.  CVS can generally
  jlt> answer these kinds of questions pretty well.

Generally; and the annotate is cool.  However, it seems this particular
code (dealing with prev) predates the CVS archive so the annotate
doesn't help much.

It does point out that the change to this function that makes prev
useless (if parm is used in ReadTitleButton) is the while loop used to
loop on prev, but now loops on parm.

Anyway, I changed the function to this now that prev serves no purpose:

    action = GetNextToken(action,&parm);
    while(parm)
    {
      action = ReadTitleButton(parm, &fl->titlebar, True, -1);
      free(parm);
      if (!action)
        break;
      action = GetNextToken(action,&parm);
    }

This still looks pretty odd to me, in particular the multiple assignment
of the action variable.  We assign it to the results of _both_
ReadTitleButton _and_ GetNextToken, and we use the results of the first
assignment as an argument to the second?

I don't pretend to understand this so maybe it's right, but it looks
odd.  Maybe someone with a better knowledge can add a comment or two for
the enlightenment of future developers :)

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 2 Dec 1998 17:50:14 +0100

> the enlightenment of future developers :)

No, that can't be right. Get next token allocates new memory and
action will be invalid after ReadTitleButton. This won't work
Try removing the assignment.

Bye

Dominik

---

From: [email-masked] (Paul D. Smith)
Date: 02 Dec 1998 14:08:32 -0500

>> action = GetNextToken(action,&parm);

  dv> No, that can't be right. Get next token allocates new memory and
  dv> action will be invalid after ReadTitleButton. This won't work
  dv> Try removing the assignment.

No, this whole set of changes is quite wrong.  It definitively breaks
most TitleStyle commands dealing with buttons.

The ReadTitleButton function _expects_ to see the entire title line
(e.g., what was prev), _not_ just the next parameter in the action
(e.g., parm).

For example, given this command:

  TitleStyle Activeup -- raised

That entire thing is supposed to be parsed by ReadTitleButton(), but the
code as changed passes _only_ the Activeup command, so it doesn't see
the rest ("-- raised").  Then it loops around again in AddTitleStyle and
tries to invoke ReadTitleButton with the --, which fails.

I think the original code was correct, basically: you pass prev to the
ReadTitleButton and assign the return value to action; ReadTitleButton
will parse as much of the action as it likes and return a pointer to the
rest.

It appears, as I mentioned in my first mail, that all the GetNextToken
is doing here is seeing if there _is_ a next token, but the value
returned is never used or useful.  I guess, based on Dominik's
subsequent comments, that might be better replaced with a PeekToken()
call?  Hmm, no, looking at that it doesn't do what I was talking about:
that function still allocates memory and returns it.  I was talking
about modifying GetNextToken() et.al. so that if you passed a NULL ptr
as the second arg it _wouldn't_ allocate any memory, but just skip the
next token.  This can be used to see if you're at the end of the action
list without going through the hassle of allocating/freeing memory.
Maybe there's a better way (a new AtEOString() function or something?)
Anyway, that's for later.  For now I'm changing the code in
AddTitleStyle() back to more-or-less what it was (plus freeing the
memory leak :).

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 3 Dec 1998 11:28:35 +0100

On Wed, Dec 02, 1998 at 02:08:32PM -0500, Paul Smith wrote:
> ...

Would you hack an enhancement request into Jitterbug?

Dominik

---

From: [email-masked] (Paul D. Smith)
Date: 03 Dec 1998 12:43:50 -0500

Message 45 (in the enhancements queue).