Re: [PATCH 0/6] PPP Patches v3

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

Hi Kristen,

> > > > And there was another problem with one variable being
> > > > guint16, but the is_option casts it back to guint8. We can't really have
> > > > that. Once you start casting on that scale the compiler will not warn
> > > > you about type mismatches or if the value of argument is too big for its
> > > > type.
> > > 
> > > Can you please let me know which git commit this fix you made was?  I 
> > > want to review it because all of the option types should only be a byte
> > > anyway, so I am trying to figure out if there was a mistake somewhere 
> > > where we were using is_option to examine a 16 bit value.  I searched
> > > through the git log and can't figure out where this change was.  I
> > > can see where you changed the option type we are comparing to a regular
> > > guint to avoid compiler problems, but not the other issue you mentioned.
> > 
> > I had a second look at these. It is the is_proto_handler actually and
> > not the is_option. However that thing applies here. A helper function to
> > find that handle would be better then manually coding g_list_find_custom
> > in the functions.
>
> so is_proto_handler was casting a 16 bit value to an 8 bit value?  Or are you
> just complaining about having g_list_find_custom in the routines.  It would
> really be easier on me to understand what you are trying to tell me if you
> you posted your patches to the list next time.

I expect you to ensure that your patches compile on 32-bit systems. And
also that they work on 32-bit and 64-bit systems. In addition I expect
that little endian and big endian works smoothly.

In general I don't wanna have casting in the source since it is a
potential trap for errors. Without casting the compiler will warn us if
we make mistakes, without we run into troubles. Especially with the
different pointer sizes it is important to not cast.

So in this specific cases, I think the usage of g_list_find_custom is
not a good idea. Manually walking this in a helper function and ensuring
that the variable and parameters types are enforced.

In short summary:

1) No casting if not really really needed. Think twice before adding a
casting to the source code.

2) Think about your optimization and understand what you are trying to
optimize. Simple code flow is preferred. Only in hot path we should
attempt to optimize manually and then it needs to be documented in the
source code itself. Otherwise lets GCC do it for us. Walking the GLib
list manually is sometimes a lot simpler to read than trying to use
foreach type functions.

3) No variable initialization on declaration. There are a few exceptions
when it makes sense. However the general rule is not to do it. We want
the compiler to warn us about variables that we might use uninitialized.
In most cases this indicates a mistake in the code flow and that the
code is too complex.

Regards

Marcel