Vahur Krouverk wrote:
> 
> > Before getting into action setting up a repository of patches it would IMO
> > be good to develop some standards concerning the patches submitted to such
> > a collection.
> [Snip]
> 
> Valid points, but I guess, that it will be better to discuss them in
> unofficial patches group, where such things should belong. I should've
> started it there, not here, so I f-up this post to there.

Good idea.  All right, let's move the discussion.  For those not reading
.programming, my suggestions are cited below:

> > - a common documentation format.
> > - rules for the source code (like marking all changes with preprocessor
> > #ifdef's)
> > - rules for modification of existing patches (bugfixes, improvements).
> > Only by the original author or by everyone?
> > - organizing frequent compiled versions for the most important platforms.
> > Like Vahur pointed out these do not necessarily have to be made by the
> > maintainers of the patch collection.
> 
> - Perhaps some coding guidelines? I know, that this opens can of worms
> and  will be possible cause for endless discussions, but it would be
> good to adopt some sensible coding standards. POV-Ray's code has evolved
> long way and it could be seen from source code: different code layout,
> different naming conventions used, some code uses tabs to ident code etc.

I don't think there is need for layout rules.  Also we would never come to
an agreement here.  Let's focus on those things really important.  

Concerning actual coding rules.  I think the most important rule is to see
how similar things are already implemented and adapt the own style to
this.  Of course there are differences between different parts of the
official POV source but i don't think we really need that precise
guidelines.  After all the patch author should be able to code his stuff
without checking against the rules every few minutes.

A few things that should be pretty obvious:

- use the POV_OSTREAM/POV_ISTREAM stuff to read and write files
- use the standard preprocessor macros (from vector.h) for vector
calculations and not own ones.
- use 'X', 'Y' and 'Z' to access the vector elements and not numbers.
- free memory you have allocated :-)
- use the POV_MALLOC etc. to allocate memory.

I think creating an exhaustive list of such things would be pretty much
work and so far most patches i have seen follow these basic rules very
well.

And finally rules only make sense if they are fairly simple and easy to
follow.  If the patch collection maintainers spend their whole time
reviewing patches and checking if the submissions comply with the rules
the whole thing won't work.

Christoph

-- 
POV-Ray tutorials, IsoWood include,                 
TransSkin and more: http://www.tu-bs.de/~y0013390/  
Last updated 13 Aug. 2002 _____./\/^>_*_<^\/\.______

Post a reply to this message

In article <3DB05F82.C97D153C@gmx.de> , Christoph Hormann 
<chr### [at] gmxde>  wrote:

> Concerning actual coding rules.

There is one rule that is more important than any other rule:

Code your patch based on the official source code or the closest possible
version of it if you need a certain functionaliyt from another patch.

Without following this rule it is next to impossible to quickly figure out
what code belongs to one patch (and how it works) from a simple visual diff.
This was one of the worst problems integrating (and removing) MegaPOV
patches into 3.5 .

    Thorsten

____________________________________________________
Thorsten Froehlich
e-mail: mac### [at] povrayorg

I am a member of the POV-Ray Team.
Visit POV-Ray on the web: http://mac.povray.org

Post a reply to this message

Thorsten Froehlich wrote:
> 
> There is one rule that is more important than any other rule:
> 
> Code your patch based on the official source code or the closest possible
> version of it if you need a certain functionaliyt from another patch.
> 

See my suggestion:

> > - rules for the source code (like marking all changes with preprocessor
> > #ifdef's)

If this is followed thoroughly it should be very clear what code belongs
to which patch.

Dependencies between the different patches could be made clear by placing
something like:

#ifdef XXX_Patch
  #define YYY_Patch
#endif

in frame.h

Christoph

-- 
POV-Ray tutorials, IsoWood include,                 
TransSkin and more: http://www.tu-bs.de/~y0013390/  
Last updated 13 Aug. 2002 _____./\/^>_*_<^\/\.______

Post a reply to this message

In article <3DB073DA.25EDB052@gmx.de> , Christoph Hormann 
<chr### [at] gmxde>  wrote:

> If this is followed thoroughly it should be very clear what code belongs
> to which patch.

Well, that makes the code harder to read when there are multiple patches,
especially when code of those patches is interleaved.  Really, MegaPOV had
that for many patches already and still later some code appeared at unusual
places that seemed to belong to patches we did not even want, either because
the #ifdefs had simply been forgotten or because one patch depended on
another and there were no #ifdefs.

That of course does not mean one should not use those #ifdefs when merging
multiple patches, but having an up-to-date patch code which only contains
the changes for that one particular patch is much easier.  In particular you
have to keep in mind that patches may not necessarily be integrated into the
exact some source code as was in the official distribution.  So the more
changes have to be tracked when merging, the more difficult it gets.
Really!


    Thorsten


____________________________________________________
Thorsten Froehlich, Duisburg, Germany
e-mail: tho### [at] trfde

Visit POV-Ray on the web: http://mac.povray.org

Post a reply to this message

Thorsten Froehlich wrote:
> 
> Well, that makes the code harder to read when there are multiple patches,
> especially when code of those patches is interleaved.  Really, MegaPOV had
> that for many patches already and still later some code appeared at unusual
> places that seemed to belong to patches we did not even want, either because
> the #ifdefs had simply been forgotten or because one patch depended on
> another and there were no #ifdefs.

I understand what you mean and that you would prefer individual patches
based on the official source, but those actually using the patches don't
want dozens of versions each implementing a single feature but a complete
version with all patches.  Merging different patches each modifying the
same code is of course a tricky task that should be made carefully.

Christoph

-- 
POV-Ray tutorials, IsoWood include,                 
TransSkin and more: http://www.tu-bs.de/~y0013390/  
Last updated 13 Aug. 2002 _____./\/^>_*_<^\/\.______

Post a reply to this message

On Fri, 18 Oct 2002 22:49:30 +0200, Christoph Hormann <chr### [at] gmxde>
wrote:
> Dependencies between the different patches could be made clear by placing
> something like:
>
> #ifdef XXX_Patch
>   #define YYY_Patch
> #endif
>
> in frame.h

I have an idea to separate that list from frame.h. I have created patches.h
(included at begining of frame.h of course) becouse it can be now included
also to pvengine.rc (resource definition) file. For example I can write in
resource definition file now:

  #ifdef SHORT_FONT_DEFINITION_PATCH
    FONT 8, "MS Sans Serif"
  #else
    FONT 8, "MS Sans Serif", 0, 0, 0x1
  #endif

  #ifdef PATCHES_BY_ABX
    BMP_SPLASH_8BPP         BITMAP  DISCARDABLE     "bitmaps\\popovwin.bmp"
  #else
    BMP_SPLASH_8BPP         BITMAP  DISCARDABLE     "bitmaps\\splash-3.5.bmp"
  #endif

ABX

Post a reply to this message