[Proj] Code style in Proj

[Kurt Schwehr]

Kristian,

Things to do when you are about to have a kid and want a distraction :)

I wonder what would be a good/interesting larger case to play with?
Preferably one with good test coverage...

ls -lSr PJ*.c | tail -15
-rw-r--r--  1 schwehr  eng   7654 Mar 20 10:49 PJ_omerc.c
-rw-r--r--  1 schwehr  eng   7659 Mar 21 07:51 PJ_cart.c
-rw-r--r--  1 schwehr  eng   7801 Mar 20 10:49 PJ_sch.c
-rw-r--r--  1 schwehr  eng   8444 Mar 20 10:49 PJ_stere.c
-rw-r--r--  1 schwehr  eng   8598 Mar 20 10:49 PJ_molodensky.c
-rw-r--r--  1 schwehr  eng   8685 Mar 29 12:41 PJ_axisswap.c
-rw-r--r--  1 schwehr  eng   9420 Mar 20 10:49 PJ_aeqd.c
-rw-r--r--  1 schwehr  eng   9907 Mar 20 10:49 PJ_deformation.c
-rw-r--r--  1 schwehr  eng  13068 Mar  6 12:50 PJ_qsc.c
-rw-r--r--  1 schwehr  eng  17186 Mar 20 10:49 PJ_unitconvert.c
-rw-r--r--  1 schwehr  eng  17405 Mar 29 12:41 PJ_pipeline.c
-rw-r--r--  1 schwehr  eng  17561 Mar 29 12:41 PJ_horner.c
-rw-r--r--  1 schwehr  eng  20639 Feb  6 08:46 PJ_helmert.c
-rw-r--r--  1 schwehr  eng  21130 Mar 20 10:49 PJ_healpix.c
-rw-r--r--  1 schwehr  eng  27407 Mar 20 10:49 PJ_isea.c

Would love to see what others come up with.

On Sun, Apr 22, 2018 at 8:10 AM, Kristian Evers <kreve at sdfe.dk> wrote:

> Hi Kurt,
>
> You are brave to start a code style discussion :-) A few comments on your
> “rules”:
>

Hah!  Far from rules.  I'm trying figure out what is even legal / works.


>
> - Combine definition and declaration
>
> It works in this example but I don’t think it can be enforced in more
> complicated code.
> Variable declarations have to be made at the start of a block and that is
> not always possible.
>
>
Agreed.  Not always possible or always a good thing.  And a style question
is do you allow variables to be defined in sub scopes?  In C++, the answer
is usually to keep things as tight/narrow as possible.  In Proj, Idonno.



> - Add const
>
> I don’t have a strong opinion on this one, although in this example I
> mostly think the “const double”
> is cluttering up the code. What is gained by adding const here?
>

Here there isn't much value, but if you come into this function cold, you
know immediately that there aren't sneaky things going on.


>
> - For double literals, have at least one digit before and after decimal
> point.  e.g. .1 -> 0.1 and 3. -> 3.0
>
> +1.
>
> - Don't have assignments hidden inside expressions
>
> Big +1!
>
> - Convert #defines to typed const values
>
> Would this be better as “static const …”?
>

In C++, I would just use constexpr.  In C, I'm guessing that for -O2 and
higher, it won't change anything.  Leaving off the static might let the
compiler be more aggressive.  But this is why using godbolt is pretty
awesome.  You can try it and see if there is any difference and, if so,
which way is more efficient.  A micro benchmark might be useful too.


>
> - IWYU - Include what you use... here math.h
>
> Another +1 here.
>
>
> Finally, I get an error when compiling your code:
>
> /Users/kevers/dev/proj4/src/PJ_august.c:25:16: error: initializer for
> aggregate is not a compile-time
>       constant [-Werror,-Wc99-extensions]
>         M * x1 * (3.0 + x12 - 3.0 * y12),
>         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
>
>
Can you reproduce that failure in godbolt, or as least, what
compiler/version/settings are you talking about?


> So for that you would have to assign xy.x and xy.y individually. Or
> possible make use of the proj_coord()
> PJ_COORD initialiser function.
>
> It would be interesting to see a more complicated function get the same
> treatment.
>
> /Kristian
>
>
> > On 22 Apr 2018, at 15:08, Kurt Schwehr <schwehr at gmail.com> wrote:
> >
> > Totally missed that lp.lam assignment!  Next iteration... also tried to
> add a const to the lp arg.
> >
> > https://godbolt.org/g/b4FUf7
> >
> > And as for the static analize, try to imagine that this is being done on
> a much more complicated function, because, yes, if a static analyzer didn't
> get this function, it's pretty much useless.
> >
> > On Sun, Apr 22, 2018 at 5:26 AM, Even Rouault <
> even.rouault at spatialys.com> wrote:
> > On samedi 21 avril 2018 15:46:22 CEST Kurt Schwehr wrote:
> > > Hi all,
> > >
> > > I've been thinking about what is possible with the Proj code base with
> an
> > > assumption that the code must be C89/C90 compatible. I played around
> for a
> > > few in godbolt with PJ_august.c (because it's small) and ended up with
> > > this. I tried to be aggressive as I could. I think my modified version
> is
> > > likely to be more static analyzer friendly.
> >
> > I hope they are intelligent enough to make sense of the original code ;-)
> >
> > > - Don't have assignments hidden inside expressions
> >
> > Definitely +1 on this !
> >
> > To avoid the modification of lp.lam in the cos(),
> >
> > > const double c = 1.0 + c1 * cos(lp.lam *= .5);
> > > const double x1 = sin(lp.lam) * c1 / c;
> >
> > this part could also be re-written, as
> >
> > const double half_lam = 0.5 * lp.lam;
> > const double c = 1.0 + c1 * cos(half_lam);
> > const double x1 = sin(half_lam) * c1 / c;
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.maptools.org/pipermail/proj/attachments/20180422/ee2c8aa0/attachment.htm