On Thu, Sep 29, 2005 at 02:26:38PM +0200, Ludwig Nussel wrote:
> Johannes Stezenbach wrote:
> > On Thu, Sep 29, 2005 Ludwig Nussel wrote:
> > > Index: dvb-apps/lib/libdvben50221/asn_1.c
> > > ===================================================================
> > > --- dvb-apps.orig/lib/libdvben50221/asn_1.c
> > > +++ dvb-apps/lib/libdvben50221/asn_1.c
> > > @@ -61,7 +61,7 @@ uint8_t *asn_1_encode(uint16_t length, u
> > >
> > > while (temp) {
> > > temp = temp >> 8;
> > > - *asn_1_words++;
> > > + (*asn_1_words)++;
> > > }
> > > if ((asn_1_array = (uint8_t *) malloc(*asn_1_words * sizeof (uint8_t))) == NULL) {
> > > printf("%s: Memory allocation failed.\n", __FUNCTION__);
> > > @@ -70,7 +70,7 @@ uint8_t *asn_1_encode(uint16_t length, u
> > > printf("%s: Allocated %d bytes.\n", __FUNCTION__, *asn_1_words);
> > >
> > > while (length) {
> > > - asn_1_array[*asn_1_words--] = length & 0xff;
> > > + asn_1_array[(*asn_1_words)--] = length & 0xff;
> > > length = length >> 8;
> > > }
> > > return asn_1_array;
> >
> > This is not "fix gcc warnings". Have you tested it?
> > (The first one looks like it fixes a bug, but without knowing what
> > the code tries to do I can't tell.)
> > Please split and send a seperate patch.
>
> I did not test it but without the fix the code doesn't make any
> sense. It would increment the _pointer_, dereference it and do
> nothing with the result. It only makes sense if the content of
> asn_1_words actually is a counter.
It is easy to see that the original code is wrong, but it is not
obvious that your change is the correct fix. That's why I ask.
> > > --- dvb-apps.orig/libs/libdvb2/notifier.c
> > > +++ dvb-apps/libs/libdvb2/notifier.c
> > > @@ -86,15 +86,14 @@ int dvb_revents(struct dvb * dvb, const
> > > */
> > > int dvb_revent(struct dvb * dvb, int fd, short revents)
> > > {
> > > - struct dvb_notifier * notifier;
> >
> > keep this
> >
> > > int i;
> > >
> > > if (revents == 0)
> > > return 0;
> > >
> > > for (i = 0; i <= dvb->nb_notifiers; ++i) {
> > > - if (dvb->notifiers.ptrs[i] == NULL ||
> > > - dvb->pollfds[i].fd != fd)
> > > + struct dvb_notifier * notifier = dvb->notifiers.ptrs[i];
> >
> > notifier = dvb->notifiers.ptrs[i];
> >
> > > + if (notifier == NULL || dvb->pollfds[i].fd != fd)
> > > continue;
> > >
> > > dvb->pollfds[i].revents = revents;
>
> It's not used outside of that loop. Same style as in dvb_revents.
OK. Personally I think it's more readable to declare variables
at the top. IIRC gcc might also create less efficient code
(two stack frame adjustments instead of one), but I'm not sure
about this...
Johannes
