[Proj] Code style in Proj
Kristian Evers
kristianevers at gmail.com
Sun Apr 22 13:42:26 EST 2018
> On 22 Apr 2018, at 20:07, Kurt Schwehr <schwehr at gmail.com> wrote:
>
> Kristian,
>
> Things to do when you are about to have a kid and want a distraction :)
Congratulations!
> I wonder what would be a good/interesting larger case to play with? Preferably one with good test coverage…
PJ_aeqd.c! There’s a bit more going on and It’s got 97.67% test coverage:
https://coveralls.io/builds/16617275/source?filename=src%2FPJ_aeqd.c <https://coveralls.io/builds/16617275/source?filename=src/PJ_aeqd.c>
>
> 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 <mailto: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.
>
At least it is done throughout the code base but definitely not consistently. I think it is fine to do so.
>
> - 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.
>
I can’t see a difference between using static and not. The assembler code is mostly gibberish to me though so I might have missed something.
Using the static keyword is probably not necessary then.
>
> - 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?
>
This is how I set up my builds:
$ CFLAGS="-std=c89 -g -Wall -Wextra -Werror -Wunused-parameter -Wmissing-prototypes -Wmissing-declarations -Wformat -Werror=format-security -Wshadow -Wfloat-conversion -O2" cmake ../../proj4/
-- The C compiler identification is AppleClang 9.1.0.9020039
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
…
Basically the same as what is done in the OSX Travis setup.
> 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 <mailto: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 <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 <mailto: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;
> >
>
> _______________________________________________
> Proj mailing list
> Proj at lists.maptools.org
> http://lists.maptools.org/mailman/listinfo/proj
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.maptools.org/pipermail/proj/attachments/20180422/b8cc4c1a/attachment-0001.htm
More information about the Proj
mailing list