From: <ams AT ludd DOT luth DOT se>
Message-Id: <200301051601.h05G1VY22333@speedy.ludd.luth.se>
Subject: Re: stubify
In-Reply-To: <3E183620.EDBE0071@phekda.freeserve.co.uk> "from Richard Dawe at
 Jan 5, 2003 01:41:52 pm"
To: djgpp-workers AT delorie DOT com
Date: Sun, 5 Jan 2003 17:01:30 +0100 (CET)
X-Mailer: ELM [version 2.4ME+ PL78 (25)]
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII
X-MailScanner: Found to be clean
X-MailScanner-SpamScore: s
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

According to Richard Dawe:
> > Index: djgpp/src/stub/stubify.c
> > ===================================================================
> > RCS file: /cvs/djgpp/djgpp/src/stub/stubify.c,v
> > retrieving revision 1.4
> > diff -p -u -r1.4 stubify.c
> > --- djgpp/src/stub/stubify.c    14 Dec 1999 12:01:36 -0000      1.4
> > +++ djgpp/src/stub/stubify.c    1 Jan 2003 18:30:10 -0000
> > @@ -231,6 +240,43 @@ void coff2exe(char *fname)
> >        perror("The error was");
> >      }
> >    }
> > +
> > +  /* Now adjust stack size if necessary, i. e. use value from environment if set. */
> > +  env_stack_size = getenv(MINSTACK_ENV_VAR);
> > +  if (env_stack_size)
> > +  {
> > +    unsigned long stack_size = 0;
> > +    char multiplier = 0;
> > +    char *stubedit_str;
> > +
> > +    if( 0 < sscanf(env_stack_size, "%li%c", &stack_size, &multiplier) &&
> > +       ( (multiplier == 0 || toupper(multiplier) == 'K' || toupper(multiplier) == 'M') ) )
> 
> 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.)

> 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.

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.

> > +    {
> > +      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?

> 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.


Right,

						MartinS