Sender: rich AT phekda DOT freeserve DOT co DOT uk
Message-ID: <3E185CAF.F72A19E9@phekda.freeserve.co.uk>
Date: Sun, 05 Jan 2003 16:26:23 +0000
From: Richard Dawe <rich AT phekda DOT freeserve DOT co DOT uk>
X-Mailer: Mozilla 4.77 [en] (X11; U; Linux 2.2.23 i586)
X-Accept-Language: de,fr
MIME-Version: 1.0
To: djgpp-workers AT delorie DOT com
Subject: Re: stubify
References: <200301051601 DOT h05G1VY22333 AT speedy DOT ludd DOT luth DOT se>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com

Hello.

ams AT ludd DOT luth DOT se wrote:
[snip]
> > What happens to the multiplier? It doesn't seem to be used.
> 
> It's a verification that the value is sane. (stubedit doesn't do this.)

OK.
 
> > Shouldn't the format be %lu%c, since stack_size is an unsigned long?
> 
> Yes.
> 
> > > +    {
> > > +      v_printf("Using " MINSTACK_ENV_VAR " = %s.\n", env_stack_size);
> > > +
> > > +      stubedit_str = malloc( sizeof("stubify ")+strlen(ofilename)+sizeof(" minstack=")+strlen(env_stack_size)+1 );
> >
> > Why do you do sizeof("stubify ")? Shouldn't it be strlen? And on "stubedit "?
> > I.e.: strlen("stubedit ")? As it turns out, the number from the above code is
> > right.
> 
> As strlen("some string") == sizeof("some string") in C, that shouldn't
> be a surprise. It's for removing an unnecessary function call when we
> can calculate it statically.

OK, but shouldn't it be sizeof("stubedit "), since that's what gets written
into the string? You could also remove the "+1" at the end, since sizeof will
include the nul terminator.
 
> Note 1. Perhaps gcc is smart enough to do this optimisation anyway. (I
> don't care.)
> 
> Note 2. Perhaps some people don't like it. (I care.)
> 
> Let me know if you oppose violently.

No, not at all! 8) I was just a bit concerned by the use of stubify instead of
stubedit in the string.

> > > +    {
> > > +      fprintf(stderr, "%s: illegal minimum stack size specified, check environment variable " MINSTACK_ENV_VAR "\n", ofilename);
> >
> > You could display the contents of the environment variable here.
> 
> Hmm. Suggest wording/layout. In particular ``printf("check environment
> variable " MINSTACK_ENV_VAR "=%s\n, env_stack_size)'' is incorrect. Or
> am I just too sensitive?

How about:

fprintf(stderr, "%s: illegal minimum stack size specified in environment
variable " MINSTACK_ENV_VAR ": %s\n", ofilename, env_stack_size);
 
> > Apart from those comments, it looks good to me. Although I hope the code
> > is readable without line wrapping in 80 columns.
> 
> Well, it not unreadable for _me_. I see now that I look, that the
> earlier code limited itself to ~80 columns. I'll do something.

It wasn't unreadable for me, either. I thought it was one of the
generally-accepted coding style rules that code should be limited to within 80
columns. (I try 74, so that diffs look OK when e-mailed.) OK, maybe I didn't
say what I meant.

But I learned a few years ago that it's pointless to get in a coding style
war. I always try to go with the "local style", whatever that is (GNU, K&R).

Bye, Rich =]

-- 
Richard Dawe [ http://www.phekda.freeserve.co.uk/richdawe/ ]