Menu
▾
▴
How to minimize code damage when adding new features. [was Re: Connect gnuplot_x11 to exterior application window]
|
From: Ethan A M. <merritt@u.washington.edu> - 2006-04-09 04:15:34
|
On Saturday 08 April 2006 07:30 pm, Daniel Sebald wrote:
> Anyway, I know that this is bloated.
> My philosophy to these patches with conditional code has always been
> that if one turns off the experimental feature it goes back *exactly*
> to the code before the patch was applied.
Right idea, but there is a better way to accomplish this.
Step 1: Figure out how you want the new feature to work, and what
might need to be added to the infrastructure to support it.
Step 2: Modify the infrastructure accordingly, confirm that everything
still works. That becomes a pre-patch.
Things in this category include
- moving routines from one source module to another because they
can no longer be static. But the routine itself is unchanged
- consolidation of variables into a shared structure, so that
the whole structure can be passed instead of 5 separate
variables. Better to change and test the revised calling
interface now, so that a bug in this sort of house-keeping change
doesn't get misinterpreted as a bug with your actual new code
features. The eventual new feature may add new fields to the
structure, but this way they will not require changes to the
various call sites or routine PROTO definitions.
- making a chunk of in-line code into a subroutine, so that it
can later be shared with new callers in your main patch
- changing manifest constants into enums or #defines
Step 3: Apply the pre-patch, and test thoroughly to insure that no bugs
have been introduced and that the original behaviour is maintained.
Or, if you are proposing to change something (new syntax, new
restrictions on the parameter values, etc) make sure it gets
tested this way in advance of your actual new feature code.
Step 4: Keep this pre-patch separate from your main patch. If it cleans
up existing code, lobby to get it in cvs now rather than waiting
for the main patch to be finalized.
Step 5: With the pre-patch in place, adding the new features should be
a much less intrusive change. You should strive for as few
conditional code locations as possible. Anywhere you end up
with suspiciously parallel code chunks separated by #if/#else/#endif,
go back and repeat steps 1 to 3.
I'm still learning this game myself, inspired by watching the development
process of the linux kernel. Have a look yourself. The amazing thing is
that the linux kernel has virtually no conditional segments despite a huge
number of configuration options; their effects are hidden in definitions,
header files, and in the choice of alternative source files to build from.
> >Here is one small example of utterly pointless conditional code:
> >
> > Add_Plot_To_Linked_List(int plot_number)
> > {
> > #ifdef EXTERNAL_X11_WINDOW
> > plot_struct *psp;
> > if (plot_number >= 0)
> > /* Make sure plot does not already exists in the list. */
> > psp = Find_Plot_In_Linked_List_By_Number(plot_number);
> > else
> > psp = NULL;
> > #else
> > /* Make sure plot does not already exist in the list. */
> > plot_struct *psp = Find_Plot_In_Linked_List_By_Number(plot_number);
> > #endif
>
> The point is that when EXTERNAL_X11_WINDOW is deactivated the code
> goes back to what it was before the patch.
Even if you think that allowing negative window numbers is not a bug,
you *still* don't need the above code sections. A less intrusive change
would be
> > Find_Plot_In_Linked_List_By_Number(int plot_number)
> > {
> > +#ifdef FOO
> > if (plot_number < 0) return NULL;
> > +#endif
The linux kernel approach would be to change the above line unconditionally
to something like
if (plot_number < MIN_PLOT_NUMBER) return NULL;
And then in some configuration header file you would have
#ifdef FOO
#define MIN_PLOT_NUMBER 0
#else
#define MIN_PLOT_NUMBER (-MAXINT)
#endif
> My thinking is that after the decision is made to remove its experimental status,
> then I or the developers could simply remove code for one of the conditions and
> if one recognizes "hey this could be simplified here or there" then do so.
The time you have the deepest understanding the code details is when you are
first creating a patch. It's far easier to clean things up at that time
then it is 3 years later when you've forgotten half of the complications
or possible side effects. The best simplification is to prepare things
so that there *is no change* to the mainline code if you back out a new
feature.
> You've inherently accepted already that the experimental code should be
> applied because if EXTERNAL_X11_WINDOW is deactivated, this change is still present.
Exactly. That is what I want to see. *First* make any small changes to the
main code, the ones that are not supposed to harm anything but might turn out
to have unforeseen side effects. Get that all sorted out and debugged, make
it a separate patch, and request that it be added to CVS before the main
patchset.
> Second thing, and it probably suggests my preference for "bottom up" style
> of programming, I see no reason to limit the routine Find_Plot... in that way.
> It's not a sanity check. As far as the linked list is concerned the plot number
> can be any integer value positive or negative, so why limit it at that level?
If that is true then you have a different kind of design flaw.
If negative id numbers are legal, but your new code uses them to signal
something else, then you have created a conflict between the two
implementations. That is bad. It would be better in that case to leave
the plot_number parameter alone and introduce a new flag or parameter
that carries whatever your new information is (I haven't looked at it
in detail to see what is now special about negative ids).
> When the new feature is deactivated, guaranteed no bug introduced because of patch.
I think you have this exactly backwards. Doing it your way in fact
maximizes the chance that trying to back it out later will fail.
As an example, let me just point out that when I tried to
deactivate either the BINARY or IMAGE parts of gplt_x11.c, it wouldn't
even compile. That is IMHO the direct result of your approach to
throwing bits of conditional code here and there at the risk of getting
some of them wrong. Particularly because there are now multiple code
paths that must be modified in parallel during any unrelated new
development work *even though only one of the code paths gets tested*.
They start out parallel, but the non-default path suffers from bit rot.
If the conditional feature is later turned off, the original code no
longer works. Better to arrange things so that the bulk of the code
is the same with or without the conditional feature.
> You are alright then with changing the name "plot" to "current_plot"
> to better reflect the meaning of the pointer, right?
I don't care one way or the other about the name. But if you want to
change the name and it removes a lot of your conditionals, then send me
a patch for the name change alone and I'll apply that right now.
Or keep it on the side as patch 1 in a series of N.
> And you think this should still have the conditional code for "new feature"
> so that there is some way of turning off the external window capability?
Yes. But as I said, I think that if the code is properly cleaned up
there will only be a handful of places where such conditional code is needed.
--
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742
|
