Hi Pedro!
On Sat, Sep 20, 2008 at 8:47 AM, Pedro Alves <[EMAIL PROTECTED]> wrote:
> Hi Pawel. Thanks for fixing this!
> Comments below. Please excuse my nit-pickiness. :-)
I'll change the patch to conform. Yet I hope you don't mind me
replying to the comments :)
>> 7.diff
>> Index: newlib/libc/stdlib/setenv_r.c
>> ===================================================================
>> --- newlib/libc/stdlib/setenv_r.c (revision 1182)
>> +++ newlib/libc/stdlib/setenv_r.c (working copy)
>> @@ -58,6 +58,9 @@
> Can you please use diff -up in your patches? Without p, it
> is harder to see what you're changing.
I'm actually using svn diff to generate the diff files. I hope svn is
capable of accepting the '-p' flag
>> ENV_LOCK;
>>
>> + // $TODO: this seems to be bogus, environment variable value
>> + // can certainly start with '=' in general case.
>> +
>> if (*value == '=') /* no `=' in value */
>> ++value;
>> l_value = strlen (value);
>
> Hmmm, interesting. Why is that there, in common code, and nobody
> has complained? It is still here in newlib HEAD. Maybe you
> should ask upstream.
Well, there that was something that caught my attention. The words in
the comment are definitely incorrect, as the value of the env variable
obviously can start with '='. May be the logic of setenv itself splits
the string, preserving the '=' from the 'a=b' pair, or may be nobody
ever had a need to store environment variables that start with '=' :)
I just wanted to raise a flag without going into details figuring out
what's going on.
>> Index: newlib/libc/sys/wince/sys/shared.h
>> ===================================================================
>> --- newlib/libc/sys/wince/sys/shared.h (revision 1182)
>> +++ newlib/libc/sys/wince/sys/shared.h (working copy)
>> @@ -30,10 +30,14 @@
>>
>> _SHMBLK _shared_init(int pgid);
>> void _shared_dump(_SHMBLK shmblk);
>> -int _shared_getshowwindow();
>> +int _shared_getshowwindow();
> Can you make this (void), while you're at it?
Umm, make what (void)?
>> void _shared_setshowwindow(_SHMBLK shmblk, int show);
>> void _shared_setenvironblk(_SHMBLK shmblk, char **env);
>> -void _shared_getenvironblk(_SHMBLK shmblk, char **env);
[skipped]
> Please, no // in C. Here an elsewhere, especially in headers. It
> breaks gcc -std=c89. If there are instances of it in headers, we should
> fix them (but separatelly). Can we make sure sentences are capitalized,
> and that there are two spaces after full stops? E.g.,
>> + int no = _shared_getenvironblk(shmblk, &buf);
>
> ^ let's be consistent and not assume c99/gnu. Please
> declare variables at the beginning of the scope not in the middle.
Sure, I can comply to c89/c99 standards. However, I'm not particularly
clear as to why, as we know what compiler we are using. May be the
compilation flags then need to enforce these standards? :)
[skipped]
>> - _shared_setenvblk(shmblk, environ);
>> + _shared_setenvironblk(shmblk, environ);
>
> What the rename? You've renamed a couple of instances, but
> left other functions still using envblk. I don't think this
> brings any clarity to the code. Can you undo that?
shared.h defines the functions as xxxenvironblk(), but shared.c
declared them as xxxenvblk(). So I make the c file declare the same
names the header file defined. Hence all the renames. Don't think it's
wrong :) I can rename the functions in the header file instead.
>> + len = s - shmblk->pginfo.environ + 1;
>> + *env = (char*)malloc(len);
> ^ cast not needed.
How so? malloc() returns void*, my understanding was you'd wanna cast
incompatible pointers...
>
>> + if (!*env) {
>> + WCETRACE(WCE_IO, "_shared_getenvironblk: FATAL ERROR
>> malloc failed for %d more bytes", len);
>
> Hmmm, should we make these fatal errors verbose even if tracing
> is not enabled.
>> + exit(1);
> I know this is already the case, but, shouldn't that be _exit or
> better yet abort instead of exit?
For both of these, I followed the existing way things are done. Things
can be changed, however, then they rather be changed all across.
>> + }
>> +
>> + memcpy(*env, shmblk->pginfo.environ, len);
>> + }
>> +
>> +
>> + return i;
> I don't see anything clearing *env, either here, of at the caller
> if this returns 0. You're calling free on this pointer
> unconditionally, which in that case, would be a dangling pointer.
I'll double check this part. My intention was :: if this function
returns (0), there was no memory allocated, otherwise it was, and the
caller is responsible for calling free().
[ skipped ]
> There are some issues with this changelog entry. We stick to the GNU format
> in our sources (which newlib also uses). Here's an example of what could
> like like:
(does anyone know how to make gmail display plain text messages in
fixed font? :) )
>
> 2008-09-18 Pawel Veselov <[EMAIL PROTECTED]>
> ^ two spaces here
>
> * newlib/libc/stdlib/setenv_r.c (_setenv_r): Flag a possible
> problem with environment variable values that start with '='.
> * newlib/libc/sys/wince/sys/shared.h (_shared_getenvironblk):
> Change return type to int. Add description.
> * newlib/libc/sys/wince/startup.c (foo): Do not reset environment
> variables from the shared block.
> * newlib/libc/sys/wince/env.c (bar): Fix shared block environment
> variables initialization.
>
> ...
>
> (_shared_setenvblk): Rename to ...
> (_shared_setenvironblk): ... this.
So, as I understood this:
* two spaces between name and email
* identify what functions had the changes in them (so what about when
the change is outside the function scope?)
* list renames (though things weren't really renamed, an incorrect
name was used)
So, there are things that I can just change, but please comment on the rest :)
Thanks!
Pawel.
--
With best of best regards
Pawel S. Veselov
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Cegcc-devel mailing list
Cegcc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cegcc-devel
