Subject: IconFill problem

From: Dan Espen <[email-masked]>
Date: Sat, 10 Apr 1999 10:11:19 -0400

Heres the original IconFill value parser:

/* Check word after IconFill to see if its "Top,Bottom,Left,Right" */
static int Get_TBLR(char *restofline,unsigned char *IconFill) {
  *IconFill = 0;                        /* init */
  if (ITIS("B") || ITIS("BOT")|| ITIS("BOTTOM")) {
    *IconFill |= ICONFILLBOT; /* turn on bottom bit */
    *IconFill |= ICONFILLHRZ; /* turn on vertical */
  } else if (ITIS("T") || ITIS("TOP")) { /* else if its "top" */
    *IconFill |= ICONFILLHRZ; /* turn on vertical */
  } else if (ITIS("R") || ITIS("RGT") || ITIS("RIGHT")) {
    *IconFill |= ICONFILLRGT; /* turn on right bit */
  } else if (!(ITIS("L") || ITIS("LFT") || ITIS("LEFT"))) { /* "left" */
    return 0;                           /* anything else is bad */
  }
  return 1;                             /* return OK */
}

And the new one:

/* Check word after IconFill to see if its "Top,Bottom,Left,Right" */
static int Get_TBLR(char *token, unsigned char *IconFill) {
  /* init */
  if (StrEquals(token, "B") ||
      StrEquals(token, "BOT")||
      StrEquals(token, "BOTTOM"))
  {
    /* turn on bottom and verical */
    *IconFill |= ICONFILLBOT;
    *IconFill |= ICONFILLHRZ;
  }
  else if (StrEquals(token, "T") ||
	   StrEquals(token, "TOP"))
  {
    /* turn on vertical */
    *IconFill |= ICONFILLHRZ;
  }
  else if (StrEquals(token, "R") ||
	   StrEquals(token, "RGT") ||
	   StrEquals(token, "RIGHT"))
  {
    /* turn on right bit */
    *IconFill |= ICONFILLRGT;
  }
  else if (StrEquals(token, "L") ||
	   StrEquals(token, "LFT") ||
	   StrEquals(token, "LEFT"))
  {
    *IconFill = 0;
  }
  else
  {
    /* anything else is bad */
    return 0;
  }

  /* return OK */
  return 1;
}

Careful examination will show that the  initialization of *IconFill at
the  top was  removed,  and I think the   intended replacement is  the
assignment in the case of parsing the "LEFT" argument.

Since the original value is not initialized, and since the other
values only turn on selected bits, this can lead to the command being
parsed with the error message:

[FVWM][ProcessNewStyle]: <<ERROR>> IconFill must specify a horizontal and vertical direction.

So the fix is to initialize on the way in, and do nothing if the word
parsed is "LEFT".

I would commit the fix myself, but the rules are code, test, commit.
Right now, all I get is core files.

I can't say I care  for the coding style, but  it seems to me that the
rule seems to be, "the last person to make a significant change wins".
Although in this case I think the  extent of the change didn't justify
changing the style.

I don't think that coding styles can be enforced when people are doing
volunteer work, and I know  how hard it  is to work  on code thats not
written "your way", so I may be complaining, but  only to suggest that
a little restraint is called for, and this may be a borderline case.

-- 
Dan Espen
444 Hoes Lane  Room RRC 1C-214             E-mail: [email-masked]
Piscataway, NJ 08854                       Phone: (732) 699-5570
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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: Sun, 11 Apr 1999 00:47:31 +0200

On Sat, Apr 10, 1999 at 10:11:19AM -0400, Dan Espen wrote:
>...
>...

OK, fixed.

> Right now, all I get is core files.

In this case the changes were so many that I had no hope to test
them alone. Therefore I committed 
 
> a little restraint is called for, and this may be a borderline case.

I didn't want to mess anything up, but in this case I simply couldn't
understand the code the way it was formatted. The result: a bug. I'm the
last person to say anything against commenting code, but to be honest:
*this* extend of comments confuses me more that uncommented code

Bye

Dominik ^_^

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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: Dan Espen <[email-masked]>
Date: Sat, 10 Apr 1999 21:01:21 -0400

> *this* extend of comments confuses me more that uncommented code

I understand, code with curly braces on separate lines, and even
worse ifs without curly braces do the same thing to me.

I often change code to the coding style I use just to understand it.

Unless we all get transplanted with the exact same brain, theres
not much I can see to do about it.

---

From: Daniel Martin <[email-masked]>
Date: Sun, 11 Apr 1999 21:03:32 -0400

> not much I can see to do about it.

I took some time out once to carefully work out all the flags to
indent that made code look more like how I wanted it to look (of
course, indent can't do everything); this went a long way towards
making other people's code readable.  Of course, I guess you'd want to 
be careful when committing CVS changes to not commit files that appear 
to be modified but in fact have only had indent run on them.
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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]