On Mon, 22 Mar 2010 20:47:16 -0700
Marcel Holtmann <marcel(a)holtmann.org> wrote:
Hi Kristen,
> > > > Changes since v2 - corrected whitespace.
> > >
> > > all the patches have been applied.
> > >
> > > So form now on please send smaller patches as soon as you have them
> > > ready and tested. If you need comments or have questions, you can use
> > > the mailing list as well.
> >
> > some additional comments from my side.
> >
> > If you have variables that are only valid inside the scope of a
> > statement, then I prefer if you only declare them there. This makes the
> > code a bit simpler to read. Check the patch I just pushed to give you an
> > example.
> >
> > Also please find access to a 32-bit system if you are running 64-bit or
> > vice versa. Our build system is strict and will turn turn all casting
> > mistakes in an error.
> >
> > Please be present on #ofono IRC as much as possible so people can report
> > such things. Remember that your code is now part of the tree and it can
> > break the build.
>
> I just started reviewing the changes you made and the first
> patch I hit I discovered that you broke something. Would you like to
> revert the patches and let me review them first, or should I just send
> a patch to the mailing list to fix what you broke?
send patches to the mailing list to fix them. The PPP code is in
development so it is just fine. I prefer to keep reverts for code that
is suppose to be stable.
If I did break something, then I am worried that the code gets too
complex. Such little changes should not have broken anything. Except I
made a stupid typo or thinking mistake (which happens of course). If it
fundamentally changes the code flow, then we do have a bigger problem.
Regards
Marcel
I wanted this code to at least be able to send a packet over the wire,
and with your changes we can't do that anymore. The problem is that
your little cosmetic change really did change the flow, simply because
you didn't think through what you were doing. Of course it is up to
you if you want to have this code be completely non-functional rather
than simply doing the revert. I'll be glad to submit a patch later
which fixes your fix and adds some comments so you know what is
actually happening.