On Saturday 20 September 2008 22:37:40, Pawel Veselov wrote:
>
> I'll change the patch to conform. Yet I hope you don't mind me
> replying to the comments :)
Not at all!
>
> >> 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
Put a file like this somewhere:
>cat gccdiff.sh
#!/bin/bash
diff=/usr/bin/diff
exec ${diff} -up "$@"
Then edit .subversion/config to point at it. I have the below:
### Set diff-cmd to the absolute path of your 'diff' program.
### This will override the compile-time default, which is to use
### Subversion's internal diff implementation.
# diff-cmd = diff_program (diff, gdiff, etc.)
diff-cmd = gccdiff.sh
> 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.
Ok, understood. I'm just sure that putting a comment there will
not raise any more bells, and it's just clutter. ;-)
> >> 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)?
Like this:
int _shared_getshowwindow(void);
In C, not specifying arguments is different from specifying
that it takes a void argument. This is a difference from c++.
In C, this happilly compiles, although it was clearly not
intended:
void foo () { }
void bar (void) { foo (1); }
>gcc foo.c -o foo.o -c -Wall -Wextra
(no output)
While this does not compile, and catches a bug:
void foo (void) { }
void bar (void) { foo (1); }
>gcc foo.c -o foo.o -c -Wall -Wextra
foo.c: In function 'bar':
foo.c:2: error: too many arguments to function 'foo'
> 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? :)
For consistency, and because headers are installed, and you
never know what people are doing with them ...
> [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.
Ok, no biggie.
> >> + 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...
This is C, not C++. You don't need to cast between/to void*.
>
> >> + 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.
Okay...
> >> + }
> >> +
> >> + 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().
Hmmm, I see, yes, I misread it, if this returns 0, you won't call free.
However, while we're at it, can shmblk->pginfo.environ be not null, but
empty (contain only the terminator), and thus this:
-void
-_shared_getenvblk(_SHMBLK shmblk, char **env)
+int
+_shared_getenvironblk(_SHMBLK shmblk, char **env)
{
char *s;
int i, len;
- if (shmblk == NULL)
- return;
+ if (!shmblk) { return; }
(BTW, in addition to my last comment, the function return
type was changed to int, but this is still returning void.
Doesn't this trigger a warning while compiling this file?)
for (i = 0, s = shmblk->pginfo.environ; *s; i++) {
- len = strlen(s);
never increments i, and returns 0? Is that expected? Should
we fetch the environment from the registry in this case,
or not? It doesn't feel like we should --- wouldn't that
be useful to be able to pass an empty environment? Hmmm,
it looks like this is already the current behaviour, and I'm not
sure we can distinguish that case from being called from a
non-cegcc app with the current interface.
>
> [ skipped ]
>
> (does anyone know how to make gmail display plain text messages in
> fixed font? :) )
I have no idea. I pull gmail messages with pop3/fetchmail, and stuff
them in an local imap server. Them I use a regular email client
to view them.
> * identify what functions had the changes in them (so what about when
> the change is outside the function scope?)
Then you make it:
* foo.c: Made bar.
If you're talking about adding a comment just above the function
to describe it, the context of the entry is still the name of
the function.
Just look at /trunk/cegcc/src/newlib/newlib/ChangeLog for
examples.
--
Pedro Alves
-------------------------------------------------------------------------
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
