[Proj] Code style in Proj

Kurt Schwehr schwehr at gmail.com
Mon Apr 30 04:51:32 EST 2018 

Here is an incomplete take on PJ_aeqd.c:

https://gist.github.com/schwehr/2a1389d8d5b711ff9796530887247d2d

On Sun, Apr 22, 2018 at 11:42 AM, Kristian Evers <kristianevers at gmail.com>
wrote:

>
>
> 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> 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> 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;
>> >
>>
>> _______________________________________________
> Proj mailing list
> Proj at lists.maptools.org
> http://lists.maptools.org/mailman/listinfo/proj
>
>
>
> _______________________________________________
> Proj mailing list
> Proj at lists.maptools.org
> http://lists.maptools.org/mailman/listinfo/proj
>



-- 
--
http://schwehr.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.maptools.org/pipermail/proj/attachments/20180430/0f70a6a5/attachment.htm

More information about the Proj mailing list